From b9eb40730b53f788d2e4bffe4ef6d9028440782e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 20 Jun 2023 20:22:28 +0200 Subject: [PATCH] fix: revert 3a2d5286084597d4c68549903709cda77dda4357 to fix 'incorrect data check' error. This error could occour in heavily threaded code for unknown reason. But maybe it's due to threads somehow not cleaning up their reused decompressor properly (maybe related to the zlib-ng version). It's strange and sad as this really costs performnace for no good reason. --- gix-pack/src/cache/delta/traverse/resolve.rs | 33 ++++++++---- gix-pack/src/data/file/decode/entry.rs | 57 +++++++++++--------- 2 files changed, 54 insertions(+), 36 deletions(-) diff --git a/gix-pack/src/cache/delta/traverse/resolve.rs b/gix-pack/src/cache/delta/traverse/resolve.rs index a4d13b3e8b0..cf4e7d87330 100644 --- a/gix-pack/src/cache/delta/traverse/resolve.rs +++ b/gix-pack/src/cache/delta/traverse/resolve.rs @@ -409,18 +409,29 @@ fn set_len(v: &mut Vec, new_len: usize) { fn decompress_all_at_once_with(b: &[u8], decompressed_len: usize, out: &mut Vec) -> Result<(), Error> { set_len(out, decompressed_len); - use std::cell::RefCell; - thread_local! { - pub static INFLATE: RefCell = RefCell::new(zlib::Inflate::default()); - } - - INFLATE.with(|inflate| { - let mut inflate = inflate.borrow_mut(); - inflate.reset(); - inflate.once(b, out).map_err(|err| Error::ZlibInflate { + // TODO: try to put this back after the next zlib-ng upgrade. + // This is from 3a2d5286084597d4c68549903709cda77dda4357 and it worked until zlib-ng-sys 1.1.9. Then it started to + // fail with `incorrect data check` 25% of the time. + // Note that thread_local! usage was also removed in two other places in `decode/entry.rs` for good measure. + // use std::cell::RefCell; + // thread_local! { + // pub static INFLATE: RefCell = RefCell::new(zlib::Inflate::default()); + // } + // + // INFLATE.with(|inflate| { + // let mut inflate = inflate.borrow_mut(); + // let res = inflate.once(b, out).map_err(|err| Error::ZlibInflate { + // source: err, + // message: "Failed to decompress entry", + // }); + // inflate.reset(); + // res + // })?; + zlib::Inflate::default() + .once(b, out) + .map_err(|err| Error::ZlibInflate { source: err, message: "Failed to decompress entry", - }) - })?; + })?; Ok(()) } diff --git a/gix-pack/src/data/file/decode/entry.rs b/gix-pack/src/data/file/decode/entry.rs index f82e33a7b9b..f850c625873 100644 --- a/gix-pack/src/data/file/decode/entry.rs +++ b/gix-pack/src/data/file/decode/entry.rs @@ -126,18 +126,21 @@ impl File { let offset: usize = data_offset.try_into().expect("offset representable by machine"); assert!(offset < self.data.len(), "entry offset out of bounds"); - use std::cell::RefCell; - thread_local! { - pub static INFLATE: RefCell = RefCell::new(zlib::Inflate::default()); - } - INFLATE.with(|inflate| { - let mut inflate = inflate.borrow_mut(); - let res = inflate - .once(&self.data[offset..], out) - .map(|(_status, consumed_in, _consumed_out)| consumed_in); - inflate.reset(); - res - }) + // use std::cell::RefCell; + // thread_local! { + // pub static INFLATE: RefCell = RefCell::new(zlib::Inflate::default()); + // } + // INFLATE.with(|inflate| { + // let mut inflate = inflate.borrow_mut(); + // let res = inflate + // .once(&self.data[offset..], out) + // .map(|(_status, consumed_in, _consumed_out)| consumed_in); + // inflate.reset(); + // res + // }) + zlib::Inflate::default() + .once(&self.data[offset..], out) + .map(|(_status, consumed_in, _consumed_out)| consumed_in) } /// Like `decompress_entry_from_data_offset`, but returns consumed input and output. @@ -149,19 +152,23 @@ impl File { let offset: usize = data_offset.try_into().expect("offset representable by machine"); assert!(offset < self.data.len(), "entry offset out of bounds"); - use std::cell::RefCell; - thread_local! { - pub static INFLATE: RefCell = RefCell::new(zlib::Inflate::default()); - } - - INFLATE.with(|inflate| { - let mut inflate = inflate.borrow_mut(); - let res = inflate - .once(&self.data[offset..], out) - .map(|(_status, consumed_in, consumed_out)| (consumed_in, consumed_out)); - inflate.reset(); - res - }) + // TODO: put this back in once we know that zlib-ng doesn't fail once in a million times (see tree-traversal) + // use std::cell::RefCell; + // thread_local! { + // pub static INFLATE: RefCell = RefCell::new(zlib::Inflate::default()); + // } + // + // INFLATE.with(|inflate| { + // let mut inflate = inflate.borrow_mut(); + // let res = inflate + // .once(&self.data[offset..], out) + // .map(|(_status, consumed_in, consumed_out)| (consumed_in, consumed_out)); + // inflate.reset(); + // res + // }) + zlib::Inflate::default() + .once(&self.data[offset..], out) + .map(|(_status, consumed_in, consumed_out)| (consumed_in, consumed_out)) } /// Decode an entry, resolving delta's as needed, while growing the `out` vector if there is not enough