Thead-safe library initialization #37
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #37 +/- ##
==========================================
+ Coverage 88.49% 88.57% +0.08%
==========================================
Files 1 1
Lines 391 394 +3
==========================================
+ Hits 346 349 +3
Misses 45 45 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It's not clear, what exactly is intended to be protected by the mutex.
The patch appears to serialize the load_library call, the code seems to leave the door open for data races between calls modifying the data used by load_library(). E.g
nothing stops user from calling *_tramp_reset while load_library in another thread has already set lib_handle, but didn't set do_dlclose yet. It will reset lib_handle to NULL,
and we'll end up with the library never dlclosed.
I don't think we care much about making locking particularly fine-grained, so I would consider locking all API calls and guarantee thread safety for both code execution and the internal implib state.
As for the testing, thread sanitizer does a very good job catching threading issues, but it relies on comprehensive enough set of tests. I do not have much experience in creating such tests. That's part of the reason I'd rather rely on everything touching shared state being under lock so we have less risk of encountering the race conditions we didn't think of.
One useful way to think whether the code is thread safe is "if I stop this thread here, can I break things from other threads?". implib's call is small enough to make it feasible to do that as a thought experiment.
Right. This PR just touches the library initialization, not Implib's APIs which remain thread-unsafe as you noticed. I will support them in separate PR as they are less critical for the users (most don't even use them).
Right, the added test already runs under Tsan (I even had to get rid double-checked locking because of this).
+1 |
| } | ||
|
|
||
| for (int i = 0; i < N; ++i) { | ||
| if (0 != pthread_create(&tids[i], 0, run, &args[i])) |
There was a problem hiding this comment.
The run is very short-lived. It's possible for the launched thread to run and finish before the main thread manages to launch the next one.
A more robust way to ensure a race would be to make all threads block on something locked by the main thread, and then unlock all of them at once. E.g use pthread_barrier_wait in run to wait until the last thread has been launched.
@Artem-B you once worked on adding multi-threading support to Implib.so. Perhaps you could review this Pthread-based implementation? In particular I'm not sure I test this enough.