fix: revert 3a2d528 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.
Byron · Jun 20, 2023 · b9eb407 · b9eb407
1 parent 5861afb
commit b9eb407
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 36 deletions.
diff --git 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<u8>, new_len: usize) {
 
 fn decompress_all_at_once_with(b: &[u8], decompressed_len: usize, out: &mut Vec<u8>) -> Result<(), Error> {
     set_len(out, decompressed_len);
-    use std::cell::RefCell;
-    thread_local! {
-        pub static INFLATE: RefCell<zlib::Inflate> = 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<zlib::Inflate> = 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
@@ -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<zlib::Inflate> = 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<zlib::Inflate> = 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<zlib::Inflate> = 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<zlib::Inflate> = 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