Conversation
Here, the data race described here in the rescuer. It's a little subtle, but the basic problem is that `defer` statements are LIFO, so what can happen is that the rescuer can be closed while there's still a log statement deferred on the stack. Because the log statements feed to `t.Logf` through a shunt, this can occasionally cause a log line to be sent to test output after the test has ended, causing the race detector to detect a race. The solution is to `defer` the close operation before the log operation. All the other maintenance services already had this race condition patched quite some time ago, but the fix didn't make it to the rescuer, probably due to an outdated branch or the like. [1] #116 (comment)
bgentry
approved these changes
Dec 17, 2023
Contributor
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Brilliant, thanks. Looks like we already did this in all the other maintenance processes but somehow forgot this one.
Is this worth a changelog entry?
Contributor
Author
|
Thanks!
TBH, probably not. In real life your logger is never going to disappear before or on the moment you stop your client, so I'm not sure you'd ever realistically run into a problem (even if the service logged a little after the client stopped, it wouldn't be harmful). I'll skip one for now for brevity sake, but lemme know if you disagree. |
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
2 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.
Here, the data race described here in the rescuer. It's a little subtle,
but the basic problem is that
deferstatements are LIFO, so what canhappen is that the rescuer can be closed while there's still a log
statement deferred on the stack. Because the log statements feed to
t.Logfthrough a shunt, this can occasionally cause a log line to besent to test output after the test has ended, causing the race detector
to detect a race.
The solution is to
deferthe close operation before the log operation.All the other maintenance services already had this race condition
patched quite some time ago, but the fix didn't make it to the rescuer,
probably due to an outdated branch or the like.
[1] #116 (comment)