Fix ThreadPrefetch close deadlock when producer blocks in parent - #1373
Open
MohammadSadeghSalehi wants to merge 1 commit into
Open
Fix ThreadPrefetch close deadlock when producer blocks in parent#1373MohammadSadeghSalehi wants to merge 1 commit into
MohammadSadeghSalehi wants to merge 1 commit into
Conversation
close() used to set a stop event, join the producer, and only then close the parent. The producer only checks the stop event at the top of its loop, so if it is blocked inside parent.__next__(), the join waits forever. That hang is deterministic for both single-level and nested ThreadPrefetch (device_put shape), 10 of 10 runs each. The fix splits cancellation from waiting. request_stop() marks the iterator closed, signals the local thread, wakes buffer waiters, and propagates non-blocking cancel down the parent chain without joining. close() calls request_stop(), then parent.close(), then joins (still skipping join when sys.is_finalizing()). __del__ calls request_stop() only, so finalizers stay best effort. next() after close() still raises ValueError on every buffer size, including 0, where a stop sentinel in an unbounded queue must not be read as end of stream. The healthy path still uses a blocking get. Measured median throughput versus base on this machine is within about 1 to 2 percent on the fast path and the slow-producer path. This is separate from google#1196. The reporter device_put script does not hang at HEAD on macOS arm64 (the is_finalizing join guard from bc17ead covers that shutdown path). A known follow-up remains: parent StopIteration is a BaseException, so the producer except Exception path does not put end of stream on the buffer for a consumer blocked in get.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
@googlebot I signed it! |
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.
Fixes #1372.
Problem
ThreadPrefetchDatasetIterator.close()sets a stop event, joins the producer, and onlythen closes the parent. The producer checks that stop event only at the top of its loop,
so when it is blocked inside
parent.__next__()the event has no effect and the joinwaits forever. The parent that could unblock it is not closed until after the join has
returned.
This is deterministic, 10 of 10 runs, for both single-level
ThreadPrefetchand thenested
ThreadPrefetch -> map -> ThreadPrefetchshape thatdevice_putproduces. It isnot a nesting-only interaction.
Change
Cancellation is split from waiting.
request_stop()marks the iterator closed, signals the local producer, wakes bufferwaiters, and propagates the same non-blocking request down the parent chain. It never
joins.
close()callsrequest_stop(), thenparent.close(), then joins. The existingsys.is_finalizing()guard on the join is preserved.__del__callsrequest_stop()only, so a finalizer never blocks on a join.Behaviour preserved
next()afterclose()still raisesValueErrorat every prefetch buffer size,including 0, where
queue.Queue(maxsize=0)is unbounded and a stop sentinel must not beread as end of stream. There is a parameterized test for this over sizes 0, 1 and 5.
Performance
The healthy path is still a blocking
get. The short timeout poll applies only after astop has been requested. Median throughput measured against the base commit on this
machine is within roughly 1 to 2 percent on both a fast producer and a slow producer,
which is inside run to run variance here.
Tests
test_close_does_not_hang_when_producer_blocked_in_parent, parameterized oversingle-level and nested prefetch. It runs in a subprocess with a hard join timeout, so a
deadlock fails the suite rather than hanging it, and it asserts child exit code, wall
time, and the absence of live
grain-thread-prefetchthreads. It fails on the basecommit on behaviour and passes here.
Question on API placement
Should
request_stop()live on the baseDatasetIterator, with a default thatpropagates to parents, rather than staying local to
ThreadPrefetch? Every iterator thatowns a worker has the same problem, and a base method would make the ownership rule
explicit:
request_stopis non-blocking cancellation,closeis the blocking path thatjoins. I kept it local here to keep the change small, and I am happy to move it if you
prefer the general shape.
Not included
Parent
StopIterationis aBaseException, so the producer'sexcept Exceptionneverputs end of stream on the buffer and a consumer blocked in
getis not woken by it. Thatis a separate defect, it is not required for the tests here, and I have left it out of
this change rather than bundling it.
Relationship to #1196
This is separate. The reporter's
device_putscript in #1196 does not hang at HEAD onmacOS arm64 on CPython 3.12, 3.13 or 3.14, and the
sys.is_finalizing()join guard frombc17eadappears to cover that shutdown path. Details are in a comment on that issue.📚 Documentation preview 📚: https://google-grain--1373.org.readthedocs.build/