fix: open() after a legacy start() left two producers feeding the queue (review of #4) - #5
Merged
Merged
Conversation
A reader started with start() runs in self and has no _worker, so open()'s _stop_previous_worker saw nothing to stop, then cleared the stop flag -- the legacy run never observed close() and kept feeding the queue next to the new worker. open() now also stops and waits for a run in self. Also make test_stop_does_not_shadow_thread_internals version-robust: Thread._stop no longer exists on Python 3.13, so assert that no Event shadows it instead of asserting the method is there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Post-merge refute review of #4.
#4's main change holds: every
open()gets its own worker, andis_alive()/join()follow it. The gap is the legacy path it explicitly supports ("a reader started the legacy way, withstart()... falls back toThread's own"):The result is two producers appending to the same queue with interleaved indices, which
_stop_previous_worker's docstring says must not happen. Before #4, this sequence raisedRuntimeError: threads can only be started once. Now it degrades silently.Fix. When there is no
_workerbutselfis running (Thread.is_alive(self)),_stop_previous_workertreatsselfas the previous worker: it signals it, waits up toreopen_timeout_s, and raises if it does not stop.Tests.
test_open_after_a_legacy_start_leaves_a_single_producer: fails on master, passes here.test_stop_does_not_shadow_thread_internalsassertedcallable(reader._stop), butThread._stopwas removed in Python 3.13, so that test fails there regardless of the code. It now asserts that noEventshadows the name, which is the property the test protects.All 7 tests pass locally on py3.10 and py3.13.
Self-reviewed only (the dispatching run disallowed sub-agents).
🤖 Generated with Claude Code
https://claude.ai/code/session_011HSBVhDjRU4apSLcRkavv9