-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure test library issues json string line-by-line #109729
Ensure test library issues json string line-by-line #109729
Conversation
r? @cuviper (rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? @pietroalbini could you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks great! A few calls can be simplified though, left a comment on one of them.
8c0f6f9
to
0a30ad8
Compare
0a30ad8
to
cd279eb
Compare
This comment has been minimized.
This comment has been minimized.
cd279eb
to
0ddde08
Compare
This comment has been minimized.
This comment has been minimized.
0ddde08
to
39e000e
Compare
@pietroalbini when you have time, can you have another look at this PR? Thank you! |
☔ The latest upstream changes (presumably #110643) made this pull request unmergeable. Please resolve the merge conflicts. |
39e000e
to
a18b750
Compare
Can this have a review please? |
Sorry for the delay! @bors r+ rollup=iffy |
☀️ Test successful - checks-actions |
Finished benchmarking commit (b4571be): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 656.048s -> 657.453s (0.21%) |
#108659 introduces a custom test display implementation. It does so by using libtest to output json. The stdout is read line by line and parsed. The code trims the line read and checks whether it starts with a
{
and ends with a}
.Unfortunately, there is a race condition in how json data is written to stdout. The
write_message
function callsself.out.write_all
repeatedly to write a buffer that contains (partial) json data, or a new line. There is no lock around theself.out.write_all
functions. Similarly, thewrite_message
function itself is called with only partial json data. As these functions are called from concurrent threads, this may result in json data ending up on the same stdout line. This PR avoids this by buffering the complete json data before issuing a singleself.out.write_all
.(#109484 implemented a partial fix for this issue; it only avoids that failed json parsing would result in a panic.)
cc: @jethrogb, @pietroalbini