Fix flaky tcp_server specs under valgrind - #104
Merged
Watson1978 merged 1 commit intoJul 31, 2026
Conversation
test_run and test_run_timeout send data, sleep TIMEOUT (10ms), then tear the server down and assert on what arrived. That assumes the reactor accepts the connection and dispatches on_read within those 10ms. Under valgrind it often does not, and the example fails with `expected: "hello", got: nil`. Seen on the Memcheck workflow, job "4.0 on ubuntu": https://github.com/socketry/cool.io/actions/runs/30615723672/job/91108332098 The other four matrix jobs passed on the same commit. Wait for the data instead of for the clock, under Timeout.timeout so the wait can never become an indefinite sleep. Five seconds is generous enough that a slow machine still passes, and a genuine hang now raises Timeout::Error at the wait itself rather than surfacing later as a confusing nil comparison. Both helpers also reset @DaTa first, so the wait condition cannot be satisfied by a value left over from an earlier example. Only these two helpers are affected. test_run_once already blocks on thread.join, and the two run_once(timeout) examples do not assert on @DaTa. Verified by shrinking TIMEOUT to 0.0001 to stand in for valgrind's slowdown, which reproduces the CI failure locally: before: 6 runs -> 4 failed (./spec/tcp_server_spec.rb:127) after: 10 runs -> 0 failed The timeout path was checked separately with a condition that never becomes true, which raises Timeout::Error at the deadline instead of hanging. rspec: 58 examples, 1 failure (detach_race_condition_spec.rb:35), a pre-existing Windows failure unrelated to this change. 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.
Problem
test_runandtest_run_timeoutsend data,sleep TIMEOUT(10ms), then tear the server down and assert on what arrived:Under valgrind the reactor is an order of magnitude slower, so 10ms is often not enough and the example fails with
expected: "hello", got: nil.Seen on the Memcheck workflow, job 4.0 on ubuntu: https://github.com/socketry/cool.io/actions/runs/30615723672/job/91108332098
The other four matrix jobs (3.2, 3.3, head, and 3.4 before fail-fast cancelled it) passed on the same commit, and the same job passes on
main, so this is timing, not a regression.Fix
Wait for the data instead of for the clock, wrapped in
Timeout.timeoutso the wait can never turn into an indefinite sleep:Five seconds is generous enough that a slow machine still passes, and a genuine hang now raises
Timeout::Errorat the wait itself instead of surfacing later as a confusingnilcomparison. Both helpers also reset@datafirst, so the wait condition cannot be satisfied by a value left over from an earlier example.Only these two helpers are affected.
test_run_oncealready blocks onthread.join, and the tworun_once(timeout)examples do not assert on@data.Verification
Shrinking
TIMEOUTto0.0001stands in for valgrind's slowdown and reproduces the CI failure locally:The timeout path was checked separately with a condition that never becomes true, which raises
Timeout::Errorat the deadline rather than hanging.rspec: 58 examples, 1 failure (detach_race_condition_spec.rb:35) — a pre-existing Windows failure unrelated to this change.🤖 Generated with Claude Code