fix: flaky test by checking ctx cancellation before racing I/O in protobuf reader/writer - #5591
Merged
Merged
Conversation
akrem-chabchoub
marked this pull request as ready for review
September 2, 2026 09:01
akrem-chabchoub
changed the title
sbackend123
approved these changes
Sep 2, 2026
akrem-chabchoub
requested review from
acud,
aloknerurkar,
gacevicljubisa,
janos and
martinconic
September 3, 2026 08:40
acud
approved these changes
Sep 3, 2026
|
|
||
| if v == nil { | ||
| t.Fatal("expected version to be non-nil") | ||
| return |
Contributor
There was a problem hiding this comment.
why do you need to return here? this is unreachable code. from go doc testing.T:
A test ends when its Test function returns or calls any of the methods
T.FailNow, T.Fatal, T.Fatalf, T.SkipNow, T.Skip, or T.Skipf. Those methods,
as well as the T.Parallel method, must be called only from the goroutine
running the Test function.
Contributor
Author
There was a problem hiding this comment.
Sorry I merged without seeing your comment.
Linter failed and prevented me from push:
7a40425
Contributor
Author
There was a problem hiding this comment.
I pull master into this branch and run make lint and it works without any issues, although nothing in master is referring to some lint changes, this is wired, didn't know why!
I reverted the changes here: #5598
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
3 participants
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.
Checklist
Description
Fixes flaky
TestIncoming_ContextTimeoutWriteMsgWithContextandReadMsgWithContextcould behave randomly when the context was already canceled.They started the I/O operation and waited to see whether the I/O finished or the context was canceled. If both happened at the same time, the program could choose either result.
This was especially easy to reproduce with the in-memory
streamteststream, where I/O could finish immediately. As a result,TestIncoming_ContextTimeoutsometimes passed and sometimes failed in CI.The fix is to check
ctx.Err()before starting the I/O. If the context is already canceled or expired, the function now immediately returns the context error every time.Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure