-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix race condition in TaskRegistry/TypeLoader when building with /mt /m mode #12653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hello @@copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo. |
The race condition occurred when multiple threads were loading types using MetadataLoadContext in /mt mode: - Thread A would create a MetadataLoadContext and store it in the static _context field - Thread B would overwrite the static _context with its own MetadataLoadContext - Thread A would try to access properties on types loaded from the now-replaced context - Thread B would dispose the _context, causing ObjectDisposedException in Thread A The fix: - Removed the static _context field that was being shared across threads - Made LoadAssemblyUsingMetadataLoadContext return both the assembly and context as a tuple - Changed GetLoadedTypeFromTypeNameUsingMetadataLoadContext to use a local context variable - Wrapped all reflection operations in a try-finally to ensure proper disposal of the context - The context is now disposed only after all reflection operations on the loaded types are complete Co-authored-by: AR-May <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a race condition in the TypeLoader class that caused System.ObjectDisposedException crashes when building with /mt /m (multi-threaded mode). The root cause was a static MetadataLoadContext _context field being shared across threads, leading to one thread disposing the context while another thread was still using types loaded from it.
Key Changes
- Removed the static
_contextfield to eliminate shared state between threads - Modified
LoadAssemblyUsingMetadataLoadContextto return a tuple containing both the assembly and its associated context - Wrapped type-finding logic in a try-finally block to ensure proper disposal of each thread's local context after metadata extraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fixes the issue appropriately. There was no reason for the context to be in a field, and the disposal pattern is corrected
Changed LoadAssemblyUsingMetadataLoadContext to return only the MetadataLoadContext instead of a tuple, allowing the calling code to use the C# 'using' statement for cleaner resource management. This addresses the code review feedback to use a using statement instead of manual disposal in finally block. Co-authored-by: AR-May <[email protected]>
The function now only creates and returns a MetadataLoadContext (it no longer loads the assembly), so the name has been updated to accurately reflect its purpose. Co-authored-by: AR-May <[email protected]>
Fixes: #12735 #12645
Context
A race condition in the TypeLoader class caused
System.ObjectDisposedExceptioncrashes when building projects with/mt /m(multi-threaded mode). The error occurred when multiple threads simultaneously loaded task types using MetadataLoadContext:Root Cause
The
MetadataLoadContext _contextfield in TypeLoader.cs (line 46) was declared as static, causing it to be shared across all threads:_contextfield_contextwith its own MetadataLoadContext_contextpropertyInfo.NameChanges Made
Made the MetadataLoadContext instance-local instead of static to eliminate the shared state:
_contextfield - Eliminated the race condition sourceCreateMetadataLoadContext- Changed fromLoadAssemblyUsingMetadataLoadContextto return onlyMetadataLoadContextto enable idiomatic C# resource management withusingstatement. The function name now accurately reflects that it only creates the context without loading assemblies.GetLoadedTypeFromTypeNameUsingMetadataLoadContext- Usesusingstatement for automatic disposal instead of manual disposal in finally block. Assembly loading now happens separately after context creation.Why This Works
The MetadataLoadContext is only used to extract metadata (property names, types, attributes) which is stored as strings/primitives during LoadedType construction. All metadata extraction completes before context disposal via the
usingstatement. The actual task execution happens in the TaskHost process where assemblies are loaded normally, so no access to disposed contexts occurs during runtime.Testing
Notes
The use of C#'s
usingstatement for resource disposal follows .NET best practices and makes the code cleaner and more maintainable compared to manual disposal in a finally block. The function naming has been updated to accurately reflect its purpose (creating a context rather than loading an assembly).💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.