Repository navigation
Do not stop the zstream loop for an interrupt - #151
Open
bruno-costanzo wants to merge 1 commit into
Open
bruno-costanzo wants to merge 1 commit into
bruno-costanzo wants to merge 1 commit into
Conversation
Each deflate() and inflate() call runs in its own rb_nogvl since ruby#88, and rb_nogvl handles pending interrupts when the call returns. So the loop does not need to stop for an interrupt and resume. The resume check from ruby#121 skipped the resume when avail_in == 0. Under Z_FINISH, deflate can still have pending output then, so Zlib::Deflate#finish could return before Z_STREAM_END and drop the end of the stream.
There was a problem hiding this comment.
🔵 Needs a closer look
The C-level change affects Ruby VM interrupt handling across all deflate and inflate paths and warrants final human review.
0 open findings
What changed in this PR
Fixes truncated deflate output when non-raising interrupts occur during stream processing.
Changes:
- Continues zstream processing after interrupts until normal termination.
- Adds regression coverage for repeated
Thread#wakeupcalls.
| File | Description |
|---|---|
ext/zlib/zlib.c |
Removes interrupt-based loop termination and retry logic. |
test/zlib/test_zlib.rb |
Verifies interrupted deflate streams remain complete. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#121 fixed the
Zlib::BufErrorfrom #57, but its retry condition letsZlib::Deflate#finishreturn before the stream ends. This is the iteration @ioquatix mentioned in #74: the same idea as #74 (@ianks), taken one step further.Problem
In zlib 3.2.4 (and in Ruby 3.4.9 and later, which ship #121 as zlib "3.2.3"), when a thread is interrupted during a deflate, the output can be truncated:
Zlib.deflatereturns truncated output and raises nothing.Zlib::Deflate#finishreturns truncated output. A latercloseraisesZlib::DataError, becausedeflateEndsees an unfinished stream.Zlib::GzipWriter#closewrites truncated output, and then usually raisesZlib::DataError.An interrupt is
Thread#wakeup, or a signal such asSIGCHLDfrom a backtick in another thread. This partly brings back Bug #10961 ("Zlib corrupts data when receive signal"), which #22 fixed with the retry.Reproduction (ruby 4.0.6, arm64-darwin, libz 1.3.1):
Zlib::BufErrorfromdeflate(Thread#wakeup leads to Zlib::BufError #57), 20 of 20 runs.Zlib::DataErrorfromclose, 20 of 20 runs. Same on Ruby 3.4.11's bundled zlib.100, 20 of 20 runs.Cause
Since #88, each
deflate()andinflate()call runs in its ownrb_nogvl, andrb_nogvlhandles pending interrupts when the call returns.zstream_run_funcstill stops its loop when the unblock function setsargs->interrupt, andzstream_run_trythen guesses whether to resume:Z_BUF_ERROR(Thread#wakeup leads to Zlib::BufError #57).avail_in > 0 && avail_out > 0. UnderZ_FINISH, deflate can still have pending output withavail_in == 0. Thenfinishreturns beforeZ_STREAM_END.Fix
The loop no longer stops for an interrupt, and the resume block is removed. Interrupts still reach Ruby after each call through
rb_nogvl. The unblock function still setsargs->interrupt, but nothing reads it now. I kept the field to make the diff small and can remove it if you prefer.Checked on this branch:
Thread#raise,Thread#kill,Timeout.timeoutand a raisingtrapstill stop deflate, inflate,Zlib.gzipandGzipWriterwithin about 60 ms.Thread#wakeupstorm or about 1,700SIGUSR1with a non-raising trap, every API gives correct output, with no slowdown: Deflate with each flush mode,Zlib.deflate,Zlib.gzip,GzipWriter,Zlib.inflate, chunked Inflate,Zlib.gunzip, andGzipReader#readand#readpartial.SIGINT/fork reproduction from Thread#wakeup leads to Zlib::BufError #57 passes 30 of 30 runs.Test
TestZlibDeflate#test_thread_wakeup_interruptdeflates 256 KiB of random data 100 times in a thread that is woken up in a loop, then inflates every output. It runs in less than 0.5 s and is pended on TruffleRuby, like the existing interrupt tests.Zlib::DataErrorfromclose. I checked this on macOS and on Linux (ruby 3.4.11, aarch64).bundle exec rake compile testpasses on Ruby 4.0.6, 3.4.8, 3.3.10 and 3.2.9 (99 tests, 639 assertions on macOS). The existingtest_thread_wakeup_interrupttests inTestZlibandTestZlibGzipReaderstill pass.Ruby's
ruby_3_4branch has #121, so it needs this change too.