Coordinate threading tests with events instead of sleeps - #1499
Open
arose26 wants to merge 1 commit into
Open
Conversation
The tests relied on sleeping half a second to land an operation in the middle of a slow sink, which only holds while the machine keeps up.
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.
Closes #1467
Four tests in
tests/test_threading.pyusedtime.sleep(0.5)to land an operation in the middle of a slow sink's write. That ordering only holds while the machine keeps up, which is the concern raised in the issue — and free-threaded builds make it less safe still.The sleeps are replaced with events that the sink sets itself, so the second thread proceeds exactly when the first is mid-operation rather than after a fixed delay.
NonSafeSinkgains two events:writing— set after the first half of the message is written, immediately before the sink's simulated I/O delaystopping— set on entry tostop(), and shareable between sinks so a test can wait for the first of a group to begin stoppingThe sink's own
time.sleep(self.sleep_time)/time.sleep(self.stop_time)stay: those are the simulated slow I/O being tested, not coordination.Why not the version proposed in the issue
As you pointed out, waiting for
logger.info()to finish would make the threads sequential and stop testing the interleaving. These events fire while the sink is still inside its delay, sologger.add()/logger.remove()still happen in parallel with an in-flight write — the same windowsleep(0.5)was aiming at, without depending on the clock.Per test:
test_safe_adding_while_loggingsleep(0.5)sink_1.writing.wait()test_safe_removing_while_loggingsleep(0.5)sink.writing.wait()test_safe_slow_removing_all_while_loggingsleep(0.5)stopping.wait()across the ten sinkstest_safe_writing_after_removingsleep(0.5)sink_1.writing.wait()The last one needed a shared event because
logger.remove()tears down ten sinks and which one stops first is not guaranteed; waiting on any single sink would reintroduce an assumption.Notes
Runtime is unchanged (~15s for the file). The removed sleeps overlapped the sinks' own delays, so this buys determinism rather than speed — worth saying since faster execution came up in the issue.
tests/test_threading.pypasses, repeated three times to check for flakiness.ruff checkandblack --checkare clean.