Bypass logcontext validity check #10401
Merged
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.
Fixes #10342
Context
This change workarounds the attempts to log assembly loads via LoggingContext that was invalidated
Theory for rootcause
The
AssemblyLoad
is AppDomain wide - and doesn't 'respect' current thread nor async context. So if we mount it in one execution context and concurrently executing code leads to assembly loading - it'll still be reported. The handler is executed synchronously - meaning the originally mounting code can be continuing execution and eventually invalidating the passed LoggingContext.Analysis Details
The added diagnostic showed the context tha was invalid was
TaskLoggingContext
, it as well showed that it was happening in msbuild within sdk (hence core version).The
TaskLoggingContext
is passed to Tracker in 2 locations:msbuild/src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs
Lines 389 to 392 in 486dbb4
This is not part of core version - so out of question for us
And
msbuild/src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs
Lines 648 to 671 in 486dbb4
Here the tracker is wrapped in using - so the LoggingContex would need to be invalidated in this scope (or passed already invalid).
TaskLoggingContext
is allways created valid (IsValid
set totrue
in constructor) and only invalidated inmsbuild/src/Build/BackEnd/Components/Logging/TaskLoggingContext.cs
Lines 125 to 136 in 486dbb4
which is called in a single place - in
ExecuteBucket
, which is outer scope forInitializeAndExecuteTask
that contains the tracker call. TheInitializeAndExecuteTask
is async - but it doesn't influence the Dispose scope.BUT - the fact that the mounting method (
InitializeAndExecuteTask
) is async suggest that there likely might be concurrently executing code.This points to tracker actually logging an assembly load event from unrelated thread - supported by the fact that the AssemblyLoadsTracker seems to be on the very 'bottom' of the stack:
While the handler should be executed synchronously - so we'd expect the
TaskBuilder
frames below the tracker frames.Simple unrelated test of trying to mount
AssemblyLoad
event in main function with simple console output, shows that the assembly loads can be reported from unrelated threads (while those 'hide' their frames, as they are not considered 'user code'):Based on the stats from builds - this was happening as well in the past - just not that often. So it's not a new regression, just some change (our or arcade) increased parallel execution or loading of assemblies on other threads.
In ideal situation we'd 'somehow' filter out the assembly load events that are from different
AsyncContext
/ExecutionContext
/SynchronizationContext
than the one requesting the tracking - but it is currently not worth the efforts.