remove redundant internal/rivertest pkg#116
Merged
Conversation
This was duplicated in a riverinternaltest package, but just a few references to it were left around and not updated. This fixes those references and nukes the redundant package.
Contributor
Author
|
Hmm, this run seems to have caught a data race in our test logging: |
Merged
brandur
approved these changes
Dec 17, 2023
Contributor
brandur
left a comment
brandur
left a comment
There was a problem hiding this comment.
Ah, crazy this was missed for so long.
brandur
added a commit
that referenced
this pull request
Dec 17, 2023
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)
Contributor
|
Race fixed in #120. |
brandur
added a commit
that referenced
this pull request
Dec 17, 2023
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)
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.
This was duplicated in a
riverinternaltestpackage after we created a publicrivertestpackage, but just a few references to it were left around and not updated. This fixes those references and nukes the redundant package.