Bugfix: avoid panic on invalid json output from libtest

[raoulstrackx]

#108659 introduces a custom test display implementation. It does so by using libtest to output json. The stdout is read and parsed; The code trims the line read and checks whether it starts with a { and ends with a }. If so, it concludes that it must be a json encoded Message. Unfortunately, this does not work in all cases:

• This assumes that tests running with --nocapture will never start and end lines with { and } characters
• Output is generated by issuing multiple write_message statements. Where only the last one issues a \n. This likely results in a race condition as we see multiple json outputs on the same line when running tests for the x86_64-fortanix-unknown-sgx target:

10:21:04 �[0m�[0m�[1m�[32m     Running�[0m tests/run-time-detect.rs (build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/run_time_detect-8c66026bd4b1871a)
10:21:04 
10:21:04 running 1 tests
10:21:04 test x86_all ... ok
10:21:04 �[0m�[0m�[1m�[32m     Running�[0m tests/thread.rs (build/x86_64-unknown-linux-gnu/stage1-std/x86_64-fortanix-unknown-sgx/release/deps/thread-ed5456a7d80a6193)
10:21:04 thread 'main' panicked at 'failed to parse libtest json output; error: trailing characters at line 1 column 135, line: "{ \"type\": \"suite\", \"event\": \"ok\", \"passed\": 1, \"failed\": 0, \"ignored\": 0, \"measured\": 0, \"filtered_out\": 0, \"exec_time\": 0.000725911 }{ \"type\": \"suite\", \"event\": \"started\", \"test_count\": 1 }\n"', render_tests.rs:108:25

This PR implements a partial fix by being much more conservative of what it asserts is a valid json encoded Message. This prevents panics, but still does not resolve the race condition. A discussion is needed where this race condition comes from exactly and how it best can be avoided.

cc: @jethrogb, @pietroalbini

[rustbot]

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

[raoulstrackx]

r? @Mark-Simulacrum

[jethrogb]

I don't fully understand the problem or the solution, but shouldn't the fix be to avoid any race conditions from occurring in the first place?

[raoulstrackx]

Fixing the race condition does not fully fix the problem here. Any test output (e.g. with --nocapture) that starts and end with { and } will result in json deserialization errors.
I've considered adding a lock or double buffer in the write_message function, but I'm not convinced that would fully fix the race condition when tests are being executed in parallel. The root of the problem is that various outputs are combined and then parsed again. In #108659 there was a discussion on an alternative approach where --logfile would be extended. I'm wondering if that would also fix this issue. If that'd be the way to go, I'd prefer to revert #108659 until the alternative approach lands.

[jyn514]

cc @pietroalbini

[pietroalbini]

@bors r+ rollup

Fixing the race condition does not fully fix the problem here. Any test output (e.g. with --nocapture) that starts and end with { and } will result in json deserialization errors.

Agreed, that was a mistake on my end here, sorry about that!

I've considered adding a lock or double buffer in the write_message function, but I'm not convinced that would fully fix the race condition when tests are being executed in parallel. The root of the problem is that various outputs are combined and then parsed again. In #108659 there was a discussion on an alternative approach where --logfile would be extended. I'm wondering if that would also fix this issue. If that'd be the way to go, I'd prefer to revert #108659 until the alternative approach lands.

So, I think here we should fix the race condition in libtest by potentially adding locking when emitting the messages. While #108659 exposes the problem to users of the Rust build system, the underlying problem of broken and overlapping JSON being emitted is a bug of libtest's --format json, and it will affect other users of it.

[bors]

📌 Commit 0d1a054 has been approved by pietroalbini

It is now in the queue for this repository.