Fix race conditions in tracing.ml #5601
Closed
Conversation
This file contains 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
It is not safe to access a global hashtable from multiple threads, even if the operations are read-only (it may be concurrently changed by another thread, which then may result in errors in the racing thread). This means we must always take the mutex, and because OCaml doesn't have a reader-writer mutex, we need to take the exclusive mutex. Eventually we should use a better datastructure here (immutable maps, or lock-free datastructures), but for now fix the datastructure that we currently use to be thread-safe. Signed-off-by: Edwin Török <[email protected]>
In preparation for OCaml 5, on OCaml 4 they'd be equivalent. Note that adding Atomic doesn't make operations on these values always atomic: that is the responsibility of surrounding code. E.g. Atomic.get + Atomic.set is not atomic, because another domain might've raced and changed the value inbetween (so in that case Atomic.compare_and_set should be used). However for global flags that are read multiple times, but set from a central place this isn't a problem. Signed-off-by: Edwin Török <[email protected]>
|
The 'tracing' module compiles, but I haven't done more testing than that on this change. |
|
Would it make sense to create a functor that wraps Hashtbl and protects its functions with a lock? Then all that we would need is updating the names and it would be re-usable. |
This was referenced May 2, 2024
robhoes
approved these changes
Jun 4, 2024
|
I believe this can be closed as the changes have already been merged as part of this #5612. |
|
Right, I see... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A quick look at tracing.ml has shown several places where a global hashtable is read without holding locks.
This is not thread safe (I probably wasn't paying attention when the original code got merged).
@GabrielBuica you can base your PR that changes
observer_modeon top of this, should be a more correct starting point.This does introduce nesting of locks though (tracer provider lock -> Spans lock), but that should be fine, I don't think it can deadlock, due to the order in which these locks are declared in the code I don't think it is possible for code in Spans to attempt to acquire the global tracer provider lock (the value is not declared yet, so it'd be a compile error).