test, ci: Fix threadsanitizer errors in mptest

[ryanofsky]

This PR fixes threadsanitizer errors that were found in bitcoin CI and reported by maflcko in bitcoin/bitcoin#33518 (comment). It also extends the libmultiprocess sanitize CI job to report the same errors.

Before this PR, libmultiprocess was built with threadsanitzer in CI, but cap'n proto and libc++ dependencies were built without it, so races in these libraries could be undetected.

Building libc++ with threadsanitzer triggered threadsanitzer errors about accessing std::cout from multiple threads. Those errors are suppressed in bitcoin CI, but there is not really a reason to suppress there here, so they are just fixed by replacing std::cout with KJ_LOG in the test. This change also cleans up mptest output, which is nice. Full output can be seen by running mptest --verbose.

Building cap'nproto with threadsanitzer triggered threadsanitzer errors in the new "thread busy" test added in #214. These were just caused by accessing some test variables from two different threads and are fixed in the last commit here.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #218 (Better error and log messages by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[ryanofsky]

Updated 1cfe541 -> 73d22ba (pr/san.1 -> pr/san.2, compare) fixing IWYU errors

[theuni]

The errors just happen because callback_thread and request_thread objects in the test are accessed by two different threads: the main test thread, and the EventLoop thread, and it is only actually safe to reference them from the EventLoop thread.

Would it be possible to add threading annotations to enforce this contract? For ex, adding a private dummy mutex, guarding the vulnerable variables with it, and holding that mutex for the duration of the EventLoop thread? We use that trick (g_msgproc_mutex) in Core to enforce a similar contract.

(Technically clang doesn't require a real mutex, only a capability, so the dummy could just be an int. But as there's no actual contention, using a real mutex seems fine imo).

[theuni]

Thanks for fixing this. This was a placeholder that I forgot to revisit.

[ryanofsky]

Would it be possible to add threading annotations to enforce this contract? For ex, adding a private dummy mutex, guarding the vulnerable variables with it, and holding that mutex for the duration of the EventLoop thread? We use that trick (g_msgproc_mutex) in Core to enforce a similar contract.

I think that trick might not apply exactly because the state here is internal to cap'nproto, and cap'nproto is single threaded and doesn't use any thread synchronization or mutexes itself. The requirement that needs to be enforced is that any capnproto function which might trigger IO (including some capnproto object destructors in this case) need to be called from the EventLoop thread with the EventLoop::sync method.

I imagine there could be compile time checks to enforce this (maybe using a pointer-like object overloading * and -> to wrap capnproto objects) but I don't have a clear idea of what it would look like. Does seem worth thinking about.

[ryanofsky]

re: #222 (comment)

Would it be possible to add threading annotations to enforce this contract?

Was thinking about this a little more, and I think an approach that could work could be to have a LoopObj<T> struct with T and EventLoop& members and * and -> operators returning T& and requiring requiring a TSA capability to dereference. The EventLoop object could be the capability object, or it could have a dummy member that's is marked as a capability. Then there could be a dummy locking object to acquire the capability, just asserting the current thread is the event loop thread when constructed.

Concretely then if the ProxyClientBase::m_client variable were declared with LoopObj<Interface::Client> type instead of Interface::Client type, the race in the #214 test wouldn't have happened because the callback_thread and request_thread variables would be protected.