Skip to content
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

Fix gh 398 hash collision mitigation #408

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jwoodmanbuildium
Copy link

@jwoodmanbuildium jwoodmanbuildium commented Dec 6, 2024

I've removed the temporary fix to mitigate hash collisions & added a new opt-in behavior for tracking instances using a concurrent dictionary & instance identifier struct.

The new optional InstanceMapBehavior has been added to the hosting extensions & container/scope constructors. The default value will maintain the integer-based hash approach. Passing a Dictionary value will update the tracked services implementation to use a concurrent dictionary in place of the hash map. When a dictionary is used, the InstanceIdentifier struct will be tracked through its IEquatable implementation, rather than the hashcode.

@jwoodmanbuildium
Copy link
Author

jwoodmanbuildium commented Dec 6, 2024

@jeremydmiller - I've went ahead and tackled this (#398) by abstracting the underlying data structure used to track the services.

@jwoodmanbuildium jwoodmanbuildium marked this pull request as ready for review December 6, 2024 19:25
@jeremydmiller
Copy link
Member

@jwoodmanbuildium Hey, there are changes here to tests that don't seem to be related. And also, #sadtrombone, there are breaking API changes. You cannot add optional arguments to an API unless you are making a full point release. You'd have to add overloads instead.

@jwoodmanbuildium
Copy link
Author

@jeremydmiller - Overloads have been added in place of adding optional arguments to the previously updated methods & constructors.

Regarding the test changes:

  • Bug_398_service_registry_hash_collisions has been deleted as it's no longer needed with the alternative instance identifier behavior
  • integration_with_aspnetcore requires an update as it's failing with the new implementation of the hostbuilder extensions. AddLamar now registers the IServiceProviderFactory using an implementation factory, and the default registered type is now being returned as IServiceProviderFactory<T> instead of the implementation type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants