Improve FileLoadException message for assembly version conflicts with loaded assembly details#123969
Improve FileLoadException message for assembly version conflicts with loaded assembly details#123969
Conversation
|
@copilot this isn't sufficient. We should know precisely what's gone wrong in this case. Let's add a new message specifically for the case that the requested assembly is not compatible with the already-loaded version. |
Changed the binder to return FUSION_E_APP_DOMAIN_LOCKED for version incompatibility instead of converting it to FUSION_E_REF_DEF_MISMATCH. The error message now precisely states: "The requested assembly version conflicts with what is already bound in the app domain or specified in the manifest." (commit 141b64f) |
|
|
||
| // TPA binder returns FUSION_E_REF_DEF_MISMATCH for incompatible version | ||
| if (hr == FUSION_E_APP_DOMAIN_LOCKED && isTpaListProvided) | ||
| hr = FUSION_E_REF_DEF_MISMATCH; |
There was a problem hiding this comment.
@copilot Is this HRESULT change observable via public APIs? If it is, it would be an unacceptable breaking change.
There was a problem hiding this comment.
You're correct - this is a breaking change. The HRESULT is observable via FileLoadException.HResult, and code exists that checks for FUSION_E_REF_DEF_MISMATCH (e.g., in Marshal.cs).
I've reverted the change. To provide a precise error message without changing the HRESULT, we would need to either:
- Store additional context (like version information) in the exception itself
- Enhance the existing FUSION_E_REF_DEF_MISMATCH message, but this is problematic because it's used for both version mismatches and other mismatches (e.g., public key token)
What approach would you recommend?
There was a problem hiding this comment.
@copilot Move fixup of FUSION_E_APP_DOMAIN_LOCKED to FUSION_E_REF_DEF_MISMATCH to the point where the HRESULT is converted to an exception. You can attach a custom error message to the exception at the point while keeping the observable HRESULT as FUSION_E_REF_DEF_MISMATCH
There was a problem hiding this comment.
Implemented your suggestion. The conversion from FUSION_E_APP_DOMAIN_LOCKED to FUSION_E_REF_DEF_MISMATCH now happens in EEFileLoadException::Throw() (not the binder), with a custom message attached from the FUSION_E_APP_DOMAIN_LOCKED resource string. The HRESULT remains FUSION_E_REF_DEF_MISMATCH to maintain compatibility. (commits 5a51868, 4da3fd6, bad6db7)
|
@copilot it sounds like we need to disconnect the production of the error message from the return code. This would allow us to keep the same return codes while producing better error messages. |
That's exactly what the current implementation does. The exception returns FUSION_E_REF_DEF_MISMATCH (0x80131040) to maintain compatibility, but the error message is disconnected from this HRESULT - it comes from a custom message stored in |
src/coreclr/vm/clrex.cpp
Outdated
|
|
||
| // Use FUSION_E_APP_DOMAIN_LOCKED's message which specifically describes version conflicts: | ||
| // "The requested assembly version conflicts with what is already bound..." | ||
| // This provides a clearer error than FUSION_E_REF_DEF_MISMATCH's generic message. |
There was a problem hiding this comment.
@copilot Please include the full version information in the new message, including the requested version and the currently loaded version.
There was a problem hiding this comment.
To include both the requested and loaded versions, I need access to the already-loaded assembly at the exception creation point. Currently, that information is only available in the binder (in the BindResult's AttemptResult) but doesn't flow to EEFileLoadException::Throw().
Would you prefer I:
- Add a new Throw overload that accepts the loaded assembly version as a parameter and thread this through the call stack?
- Look up the already-loaded assembly in
CreateVersionConflictExceptionusing AppDomain's assembly cache? - Store the loaded assembly info in a different mechanism (e.g., thread-local storage) that can be accessed when creating the exception?
What approach would be most acceptable?
Add a new resource string that takes the loaded assembly path as a separate format argument, producing cleaner messages like: Loaded assembly: 'Foo, Version=1.0.0.0' (loaded from 'C:\bar\Foo.dll') Instead of threading pre-formatted strings, return the conflicting Assembly* via a new optional out-parameter on BindUsingPEImage. The caller extracts name and path directly, selecting between the 2-arg and 3-arg format strings based on path availability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add LoadFromAssemblyPath_MvidMismatch_ErrorMessageIncludesPathAndVersion to AssemblyLoadContextTest validating that the FileLoadException message includes the loaded assembly's version and file path. Also add a v2 test assembly project (same name, version 2.0.0.0) and update the binder tracing test to check for the path in the error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1. Change 'A different version of the assembly' to 'A different copy of assembly' in resource strings, since MVID mismatch can occur between assemblies with the same version but different content. 2. Include INCLUDE_PUBLIC_KEY_TOKEN in the GetDisplayName call in assemblynative.cpp to match bindertracing.cpp, so the exception message and ETW trace show consistent assembly identity details. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fba813b to
3380638
Compare
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch MVID mismatch tests from VersionDowngrade to ReferencedClassLib (a TPA assembly) and add a ReferencedClassLibVersion2 project for the v2 copy. Add three test scenarios: - Custom ALC: load ReferencedClassLib then a different copy - Default ALC with TPA assembly: ReferencedClassLib is already in TPA, load a higher-version copy with different MVID - Default ALC with non-TPA assembly: load VersionDowngrade v1 then v2 into Default ALC via RemoteExecutor Add an optional out parameter to BindAssemblyByNameWorker that returns the existing assembly from the execution context when the by-name bind fails. In DefaultAssemblyBinder::BindUsingPEImage, when the TPA version check fails but an existing assembly is found, perform the MVID comparison directly and provide the improved error message. Previously, the failure cache entry from the version check would prevent the MVID check in AssemblyBinderCommon::BindUsingPEImage from being reached. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d chain Thread an optional Assembly** out parameter through BindLocked, BindByName, and BindAssembly so the conflicting assembly is passed out directly at the point where the version mismatch is detected. This covers both cases: - Assembly already in the execution context (FindInExecutionContext) - Assembly freshly loaded from TPA (BindByTpaList) BindAssemblyByNameWorker now passes through to BindAssembly instead of doing a redundant ExecutionContext lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the FUSION_E_APP_DOMAIN_LOCKED to FUSION_E_REF_DEF_MISMATCH conversion from the shared BindLocked into DefaultAssemblyBinder's BindAssemblyByNameWorker, since this is TPA-specific behavior that does not belong in shared binder code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…mage Replace the MVID comparison with a check for FUSION_E_REF_DEF_MISMATCH from BindAssemblyByNameWorker, which already detects the version conflict. Preserve the original HRESULT in the thrown exception. In assemblynative.cpp, check for the existing assembly out-param first to decide whether to show the improved error message, rather than checking the HRESULT. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use COMPlusThrowHR instead of COMPlusThrow so the original HRESULT (e.g. FUSION_E_REF_DEF_MISMATCH for TPA version conflicts, COR_E_FILELOAD for MVID mismatches) is preserved in the managed FileLoadException. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The parameter is now also used for version conflicts (not just MVID mismatches), so the name should reflect the broader usage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
That check was from a previous change within this PR. It is not in main - the final diff against main here didn't remove any check there. |
Assert that COR_E_FILELOAD is preserved for MVID mismatches (custom ALC and non-TPA Default ALC) and FUSION_E_REF_DEF_MISMATCH is preserved for TPA version conflicts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
be9ffd2 to
3d944dc
Compare
There was a problem hiding this comment.
Pull request overview
Improves FileLoadException messaging for assembly load failures caused by version/MVID conflicts by including details about the already-loaded assembly (full name/version and, when available, file path).
Changes:
- Threads an optional “existing assembly on conflict” out-parameter through
BindUsingPEImageand related binder helpers. - Enhances CoreCLR error message construction (and binder tracing) to include the loaded assembly’s identity and path for conflict scenarios.
- Updates/adds tests and test assets to validate the new error message content and tracing output.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/binder/assemblybindercommon.cpp | Threads existing-assembly info through bind helpers and returns it on conflict (notably for MVID mismatch). |
| src/coreclr/binder/defaultassemblybinder.cpp | Propagates existing assembly details for TPA version conflicts so callers can produce richer errors. |
| src/coreclr/binder/customassemblybinder.cpp | Passes through the optional existing-assembly-on-conflict out-parameter. |
| src/coreclr/binder/bindertracing.cpp | Extends tracing to include loaded assembly identity/path for MVID mismatch-related failures. |
| src/coreclr/vm/assemblybinder.h | Updates BindUsingPEImage signature to support returning the conflicting loaded assembly. |
| src/coreclr/vm/assemblynative.cpp | Builds improved FileLoadException messages using existing assembly full name and path when available. |
| src/coreclr/dlls/mscorrc/resource.h | Adds resource IDs for the new detailed “already loaded” messages. |
| src/coreclr/dlls/mscorrc/mscorrc.rc | Adds the new localized message templates used to format detailed conflict info. |
| src/tests/Loader/binding/tracing/BinderTracingTest.ResolutionFlow.cs | Updates tracing test expectations to validate the new error message detail. |
| src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj | Adds shared HResults include and embeds new test assemblies needed for conflict scenarios. |
| src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Test.AssemblyVersion2/System.Runtime.Loader.Test.AssemblyVersion2.csproj | Adds a v2 assembly variant used to provoke version/MVID conflicts in tests. |
| src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Test.AssemblyVersion1/System.Runtime.Loader.Test.AssemblyVersion1.csproj | Adjusts versioning properties to align with the new version-conflict test setup. |
| src/libraries/System.Runtime.Loader/tests/ReferencedClassLibVersion2/ReferencedClassLibVersion2.csproj | Adds a v2 referenced class library to create default-ALC TPA vs non-TPA conflict scenarios. |
| src/libraries/System.Runtime.Loader/tests/ReferencedClassLib/ReferencedClassLib.csproj | Sets an explicit v1 AssemblyVersion for baseline comparison in version-mismatch tests. |
| src/libraries/System.Runtime.Loader/tests/AssemblyLoadContextTest.cs | Adds new tests validating enriched conflict messages and expected HRESULTs across ALC/TPA scenarios. |
🤖 Copilot Code Review — PR #123969Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: This PR improves diagnostic information for assembly binding conflicts — the existing error messages were vague ("Assembly with same name is already loaded") and the original HRESULT was lost, making it difficult to troubleshoot assembly loading issues. The motivation is clear and well-justified. Approach: Sound approach — threading the conflicting assembly through the bind chain captures it at the detection point (rather than a separate post-hoc lookup), and using Summary: Detailed Findings
|
| Test | Scenario | Expected HResult |
|---|---|---|
LoadFromAssemblyPath_CustomAlc_VersionMismatch |
MVID mismatch in custom ALC | COR_E_FILELOAD |
LoadFromAssemblyPath_DefaultAlc_Tpa_VersionMismatch |
TPA version conflict | FUSION_E_REF_DEF_MISMATCH |
LoadFromAssemblyPath_DefaultAlc_NonTpaAssembly_MvidMismatch |
Non-TPA MVID mismatch (via RemoteExecutor) | COR_E_FILELOAD |
Each test validates both the error message content (simple name, loaded assembly display name, path) and the specific HResult. The non-TPA test correctly uses RemoteExecutor to isolate the Default ALC state.
The test infrastructure additions (AssemblyVersion2, ReferencedClassLibVersion2) are well-structured — ReferencedClassLibVersion2 shares source with ReferencedClassLib but builds with version 2.0.0.0, and the explicit AssemblyVersion on ReferencedClassLib.csproj ensures a stable version baseline.
💡 CustomAssemblyBinder Now Keeps FUSION_E_APP_DOMAIN_LOCKED — Verify intent
File: src/coreclr/binder/assemblybindercommon.cpp
The old code converted FUSION_E_APP_DOMAIN_LOCKED → FUSION_E_REF_DEF_MISMATCH in shared BindLocked when isTpaListProvided was true. Since custom ALCs don't have TPA lists, this conversion never fired for them. The new code moves it to DefaultAssemblyBinder::BindAssemblyByNameWorker only. For custom ALCs, version mismatches go through AssemblyBinderCommon::BindUsingPEImage's MVID check path instead, which produces COR_E_FILELOAD. This maintains the same effective behavior — just confirming the two paths are functionally equivalent for custom ALCs.
Generated by Code Review for issue #123969 · ◷
Description
When Assembly.Load fails due to a version mismatch (requesting v9.0 when v8.0 is loaded), the exception message is uninformative:
This change improves the error message to include specific details about both the requested assembly and the already-loaded assembly, including versions and file paths, while maintaining backward compatibility.
Solution
Thread the loaded assembly information from the binder through
BindUsingPEImagevia an optional out-parameter. When an assembly load fails due to version/MVID mismatch, construct a detailed error message inassemblynative.cppthat includes:Changes
Modified Files:
src/coreclr/binder/assemblybindercommon.cpp:BindUsingPEImageto accept optionalSString* pExistingAssemblyNameout-parameterpExistingAssemblyout-parameter when version/MVID conflict detectedsrc/coreclr/binder/customassemblybinder.cpp&defaultassemblybinder.cpp:BindUsingPEImagesignatures to include the new out-parametersrc/coreclr/binder/bindertracing.cpp:src/coreclr/vm/assemblybinder.h:BindUsingPEImagesignaturesrc/coreclr/vm/assemblynative.cpp:BindUsingPEImagesrc/coreclr/dlls/mscorrc/mscorrc.rc&resource.h:IDS_HOST_ASSEMBLY_RESOLVER_ASSEMBLY_ALREADY_LOADED_WITH_VERSION: Error message with loaded assembly versionIDS_HOST_ASSEMBLY_RESOLVER_ASSEMBLY_ALREADY_LOADED_WITH_VERSION_AND_PATH: Error message with loaded assembly version and file pathTests:
AssemblyResolutionDowngradeTest.csto validate new error message formatSystem.Runtime.Loader.Test.AssemblyVersion2test projectBinderTracingTest.ResolutionFlow.csto handle new tracing outputResult
Before:
After (with path):
After (without path):
The error message now includes:
Backward Compatibility
✅ No breaking changes:
FileLoadException.HResultreturns the same HRESULT values as beforeBindUsingPEImagesignature change is internal and not exposed via public APIsTesting
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.