Rename thread-local to fiber-local in concurrent context #1984
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1984 +/- ##
=======================================
Coverage 97.71% 97.71%
=======================================
Files 1004 1004
Lines 50628 50639 +11
=======================================
+ Hits 49472 49483 +11
Misses 1156 1156
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| # @see https://ruby-doc.org/core-3.1.2/Thread.html#method-i-5B-5D Thread attributes are fiber-local | ||
| class FiberLocalContext | ||
| # FiberLocalContext can be used as a tracer global reference to create | ||
| # a different {Datadog::Tracing::Context} for each fiber. In synchronous tracer, this |
There was a problem hiding this comment.
Minor: I'm curious about what we mean here by "synchronous tracer"...?
There was a problem hiding this comment.
Yeah, it reads kind of weird.
I rephrased it to actually document some information that would be useful when reading this class.
| # FiberLocalContext can be used as a tracer global reference to create | ||
| # a different {Datadog::Tracing::Context} for each fiber. In synchronous tracer, this | ||
| # is required to prevent multiple fivers sharing the same {Datadog::Tracing::Context} |
There was a problem hiding this comment.
Minor: We repeat the same docs for the class and the initializer. I suggest perhaps removing the extra copy on the constructor.
There was a problem hiding this comment.
Removed duplication from initializer.
| context = fiber_local_context.local | ||
|
|
||
| Fiber.new do | ||
| expect { @fiber_context = fiber_local_context.local } | ||
| .to change { thread_contexts.size }.from(0).to(1) | ||
|
|
||
| expect(@fiber_context).to be_a Datadog::Tracing::Context | ||
| end.resume | ||
|
|
||
| expect(@fiber_context).to_not eq(context) |
There was a problem hiding this comment.
Minor: The instance variable use seems a bit weird to me, and I don't quite like that if somehow the fiber doesn't assign to @fiber_context, the expect(@fiber_context).to_not eq(context) will still pass (because it'll be nil).
So here's a tiny suggestion that I think covers both 😄:
| context = fiber_local_context.local | |
| Fiber.new do | |
| expect { @fiber_context = fiber_local_context.local } | |
| .to change { thread_contexts.size }.from(0).to(1) | |
| expect(@fiber_context).to be_a Datadog::Tracing::Context | |
| end.resume | |
| expect(@fiber_context).to_not eq(context) | |
| main_fiber_context = fiber_local_context.local | |
| other_fiber_context = nil | |
| Fiber.new do | |
| expect { other_fiber_context = fiber_local_context.local } | |
| .to change { thread_contexts.size }.from(0).to(1) | |
| end.resume | |
| expect(other_fiber_context).to be_a Datadog::Tracing::Context | |
| expect(other_fiber_context).to_not eq(main_fiber_context) |
There was a problem hiding this comment.
Nice improvement! I applied it to both places. 🙇
|
|
||
| Fiber.new do | ||
| expect { @fiber_context = fiber_local_context.local } | ||
| .to change { thread_contexts.size }.from(0).to(1) |
There was a problem hiding this comment.
Minor: Should thread_contexts be renamed as well?
There was a problem hiding this comment.
I wasn't 100% sure if it would make sense to rename thread_contexts, given the values do come from threads, but they happen to match with the respective fibers.
I ended up renaming it now, take a look :)
💯
|
Great, thanks! |
Fixes #1979
Thread[]andThread[]=are fiber-local attributes¹, not thread-local as we have described them inddtrace.But they are also thread-safe, given these attributes are retrieved from the "current thread's root fiber if not explicitly inside a Fiber". Effectively,
Thread[]andThread[]=are both thread-safe and fiber-safe, but fiber-safe is the most accurate way to describe them.This PR does not change any behavior, only clarifies nomenclature around fiber- vs thread-local storage.
Also, this PR adds one test to ensure the fiber-local behavior explicitly, given all tests are centered around threads today.
One alternative to this PR is to move from using
Thread[]toThread#thread_variable_get, making the execution context thread-safe but not fiber-safe. I believe this is not in our best interest, given in a context where Fibers are utilized concurrent spans could be created and interwoven, creating unexpected span child-parent relationships. Keeping our contexts fiber-local avoids this issue but creating distinct execution contexts for each fiber.