Prevent ActivityId leak across threads #55625
Conversation
Fixes dotnet#51608 Previously running an async task with two or more levels of nesting on the EventSource activity stack would leave the ActivityID set on the old thread after being swapped out at a yield point. This meant that threadpool threads would often have stale ActivityIDs present when they began a new work item. This in turn produced non-sensical logs where it appeared that unrelated work items were nested within each other. Most of this change is adding tests. The ActivityIdIsZeroedOnThreadSwitchOut test is the one being fixed. The remainder of the new tests pass both before and after the change. I added those to better describe some of the existing behavior and add a little confidence that the change didn't have unintended effects elsewhere. As best I can tell the Activity tracking feature didn't have any testing previously and there is certainly still room for improvement on test coverage.
|
Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti Issue DetailsFixes #51608 Previously running an async task with two or more levels of nesting on the EventSource activity stack would leave the ActivityID set on the old thread after being swapped out at a yield point. This meant that threadpool threads would often have stale ActivityIDs present when they began a new work item. This in turn produced non-sensical logs where it appeared that unrelated work items were nested within each other. Most of this change is adding tests. The ActivityIdIsZeroedOnThreadSwitchOut test is the one being fixed. The remainder of the new tests pass both before and after the change. I added those to better describe some of the existing behavior and add a little confidence that the change didn't have unintended effects elsewhere. As best I can tell the Activity tracking feature didn't have any testing previously and there is certainly still room for improvement on test coverage. @dotnet/dotnet-diag - can someone kindly review this?
|
- stop leaking the ActivityEventSource - disable the tests in the browser where Activity tracking isn't enabled
src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/ActivityTracking.cs
Show resolved
Hide resolved
| // Are we popping off a value? (we have a prev, and it creator is cur) | ||
| // Special case only relevant for mixed SetActivityID usage: | ||
| // | ||
| // Are we MAYBE popping off a value? (we have a prev, and it creator is cur) |
There was a problem hiding this comment.
So this refers to the case where get scheduled on the same thread, but with some execution context such that it was the parent scope of the context we were in, yet not necessarily from popping the event nesting?
There was a problem hiding this comment.
Yep : ) For example something like this:
async Task A()
{
Task b = Task.Run(B);
await Task.Yield();
}
async Task B()
{
EventSource.Log.FooStart(); // this pushes a new activity
await Task.Yield(); // assume that when this yields the scheduler decides to resume running A() on this thread.
// A's current activity is one level less nested than the current activity in B()
EventSource.Log.FooStop();
}
ghost
locked as resolved and limited conversation to collaborators
Fixes #51608
Previously running an async task with two or more levels of nesting on the EventSource activity stack would leave the ActivityID set on the old thread after being swapped out at a yield point. This meant that threadpool threads would often have stale ActivityIDs present when they began a new work item. This in turn produced non-sensical logs where it appeared that unrelated work items were nested within each other.
Most of this change is adding tests. The ActivityIdIsZeroedOnThreadSwitchOut test is the one being fixed. The remainder of the new tests pass both before and after the change. I added those to better describe some of the existing behavior and add a little confidence that the change didn't have unintended effects elsewhere. As best I can tell the Activity tracking feature didn't have any testing previously and there is certainly still room for improvement on test coverage.
@dotnet/dotnet-diag - can someone kindly review this?
@brianrob - could you review this?