Fix hash collision in TypeMapLazyDictionary causing non-deterministic ArgumentException #123502
Conversation
…nstead of hash codes Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
|
@copilot Please remove the added test. It doesn't really provide any protection here. |
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
|
@copilot Please revert all changes under tests/Interop/TypeMap. There is little value to the added tests outside of what is already present. The failure case this PR represents doesn't represent an obvious test failure but rather a poor design decision. |
Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Reverted all test changes in commit 0e5dc16. Only the core fix in TypeMapLazyDictionary.cs remains. |
|
/backport to release/10.0 |
|
Started backporting to |
There was a problem hiding this comment.
Pull request overview
This PR fixes a non-deterministic ArgumentException in TypeMapping.GetOrCreateExternalTypeMapping caused by hash collisions when external type names shared the same string.GetHashCode() value. The change updates the lazy external type map to use the full string key instead of its hash code, eliminating the possibility of collisions in this path.
Changes:
- Updated
LazyExternalTypeDictionaryto useDictionary<string, DelayedType>instead ofDictionary<int, DelayedType>. - Removed the now-unnecessary
ComputeHashCodehelper and all call sites. - Adjusted
TryGetOrLoadTypeandAddto use the string key directly for lookups and duplicate checks.
AaronRobinsonMSFT
deleted the
copilot/fix-argumentexception-duplicate-key
branch
…n-deterministic ArgumentException (#123517) Backport of #123502 to release/10.0 /cc @AaronRobinsonMSFT @Copilot ## Customer Impact - [x] Customer reported - [ ] Found internally When large numbers of `TypeMapAssociationAttribute` exist, it can occur that collisions of type names (`string`s) occur. The current approach ignored the possibility of collision and simply acquired the result of `GetHashCode()` and inserted that as a key into a `Dictionary<,>`. This meant there was no opportunity to handle the case when two non-equal `string` instances generate the same hash code. Using the hash code directly was done to avoid the non-trivial increase in memory usage imposed by using the `string` instances as the key. The solution is to use the `string` as the key instead. ## Regression - [ ] Yes - [x] No ## Testing Verified using a CsWinRT repro case. Prior to this fix a collision would occur 1 / 100 times run. After this change the repro ran for more than 1000 iterations with no collisions. ## Risk Low. This new approach does increase the baseline amount of memory when the `TypeMap` is used on coreclr. This was deemed acceptable since the coreclr isn't considered the P1 scenario for `TypeMap`. The native AOT, the P1 scenario for `TypeMap`, is not impacted by this change. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
Description
TypeMapping.GetOrCreateExternalTypeMappingthrew non-deterministicArgumentExceptionwhen type names hashed to the same value. For example,"Windows.Media.Devices.Core.FrameFlashControl"and"Windows.Security.Credentials.PasswordCredential"produce identicalGetHashCode()results, causing collision on insertion.Root cause:
LazyExternalTypeDictionaryusedDictionary<int, DelayedType>keyed bystring.GetHashCode()instead of the string itself.Changes
LazyExternalTypeDictionaryto useDictionary<string, DelayedType>ComputeHashCodemethodTryGetOrLoadTypeandAddThis matches the collision-safe approach already used in
LazyProxyTypeDictionarybut simplified since external type mappings don't require multi-value support.The fix addresses a design issue where hash collisions could occur. The existing test coverage is sufficient to validate the behavior.
Original prompt
This section details on the original issue you should resolve
<issue_title>TypeMapping.GetOrCreateExternalTypeMapping throws ArgumentException for duplicate key on CoreCLR occasionally</issue_title>
<issue_description>### Description
In the CsWinRT 3.0 CI, we have been occasionally seeing
TypeMapping.GetOrCreateExternalTypeMappingthrowArgumentException. This seems to happen for different types and reruns typically cause it to pass. I have also been able to repro it locally a couple times but took multiple runs of the same exe.It does seem to happen only on CoreCLR and we haven't been able to repro it with AOT. Inspecting the assemblies doesn't show any duplicate attributes for the TypeMap. The specific typemap entry from the exception stack in this instance is in
WinRT.Interop.dll.Reproduction Steps
Repro and traces
Run ClassActivation.exe until repro
Expected behavior
No exception when there is no duplicate typemap entry
Actual behavior
Regression?
No response
Known Workarounds
No response
Configuration
.NET 10
x86/x64 CoreCLR
Other information
No response</issue_description>
<agent_instructions>Please address the issue based on details in the comment #123337 (comment)</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@manodasanW FYI @AaronRobinsonMSFT @jkoritzinsky @AaronRobinsonMSFT I can repro this with the attached `ClassActivation.exe` case. I'll take a look today. @AaronRobinsonMSFT @manodasanW I think the TypeMap failure is a red-herring. There is an exception prior to that that seems to cause an issue. I suppose it is possible it poisoned the cache and then succeeds later; that is just a guess. It would be helpful if you could verify the first exception is expected because it only seems to happen when the TypeMap fails, so I assume it is the underlying issue.I'm using the
createdumpcollection tool to help find the original exception.Set the following to use the
createdumptool.