-
-
Notifications
You must be signed in to change notification settings - Fork 450
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
Locking in ProviderGraph to prevent duplicate code generation #3109
Locking in ProviderGraph to prevent duplicate code generation #3109
Conversation
felixkmh
commented
Apr 2, 2024
•
edited
Loading
edited
- Implemented fix as discussed in Duplicate Type Generation #3083
- Added single lock object to synchronize code generation
- Added unit test to cover this fix
- Confirm that the project builds
- All automated tests succeed
@@ -23,7 +24,10 @@ public ProviderGraph(StoreOptions options) | |||
|
|||
public void Append<T>(DocumentProvider<T> provider) where T : notnull | |||
{ | |||
_storage = _storage.AddOrUpdate(typeof(T), provider); | |||
lock (_storageLock) |
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.
Don't do the lock here, it's unnecessary w/ the immutable hash map
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'm not sure how ImHashMap
s work exactly, but the reason I also locked it is because I thought that if two threads enter this method concurrently, then they call AddOrUpdate
on the same instance. Then they each would create a new map based on that same map instance, resulting in two separate instances that do not contain the item the other thread tried to add. The assignment only happens after AddOrUpdate
returns, so whoever assigns to _storage
last "wins".
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.
Maybe I'll just add that as another test case and see if that can happen. Although tests of that nature aren't the most reliable.
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.
@felixkmh Thanks for taking this on! |