fix(client): honour an already-cancelled context before sending - #30
Merged
Conversation
request marshalled the frame, registered the pending channel and wrote the whole thing to the server before it ever looked at ctx, so a caller whose deadline had already blown still enqueued the job. The select at the bottom cannot cover for that. By the time it runs the response may already be sitting in respCh, and when both cases of a select are ready Go picks between them uniformly at random, so an expired context loses roughly half the time. That is what TestClient_ContextTimeout caught on ubuntu CI, where the read goroutine gets to deliver the response before the requesting goroutine reaches the select. It never reproduced on darwin: 400 runs under load came back clean, because locally the round trip is always slower than the few microseconds between writeFrame returning and the select. Forcing the response to land first, by sleeping in between, failed 27 times in 40. With the check in place that same forced ordering failed 0 in 40. All nine client calls across job, workflow and subscription route through request, so they all pick this 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
1 participant
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.
request marshalled the frame, registered the pending channel and wrote the whole thing to the server before it ever looked at ctx, so a caller whose deadline had already blown still enqueued the job. The select at the bottom cannot cover for that. By the time it runs the response may already be sitting in respCh, and when both cases of a select are ready Go picks between them uniformly at random, so an expired context loses roughly half the time.
That is what TestClient_ContextTimeout caught on ubuntu CI, where the read goroutine gets to deliver the response before the requesting goroutine reaches the select. It never reproduced on darwin: 400 runs under load came back clean, because locally the round trip is always slower than the few microseconds between writeFrame returning and the select. Forcing the response to land first, by sleeping in between, failed 27 times in 40. With the check in place that same forced ordering failed 0 in 40.
All nine client calls across job, workflow and subscription route through request, so they all pick this up.