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

Make OptionTypeConverter thread-safe #6891

Merged
merged 2 commits into from Feb 13, 2024
Merged

Conversation

cmeeren
Copy link
Contributor

@cmeeren cmeeren commented Feb 13, 2024

Follow-up of #6883.

I realized that using Dictionary with double-checked locking means that it's possible one thread calls TryGetValue while another thread modifies the dictionary, which AFAIK is not allowed for Dictionary. This PR uses a ConcurrentDictionary instead, which avoids that issue.

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c5a043e) 73.29% compared to head (00fab63) 73.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6891      +/-   ##
==========================================
+ Coverage   73.29%   73.32%   +0.03%     
==========================================
  Files        2584     2584              
  Lines      128343   128343              
==========================================
+ Hits        94069    94109      +40     
+ Misses      34274    34234      -40     
Flag Coverage Δ
unittests 73.32% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michaelstaib michaelstaib changed the title Make OptionTypeConverter more thread-safe Make OptionTypeConverte thread-safe Feb 13, 2024
@michaelstaib
Copy link
Member

OK.. I did not review the F# PRs so far ...

You would not use a lock in combination with a ConcurrentDictionary.
GetOrAdd(TKey, TValue)

The locking implemented with the ConcurrentDictionary is far more efficient as the lock on the buckets in the dictionary.

@cmeeren
Copy link
Contributor Author

cmeeren commented Feb 13, 2024

ConcurrentDictionary guarantees "only" consistency during reads/updates. The locking prevents doing the work of generating the value for a key more than once in case there are multiple threads trying to generate the value for the same key at the same time.

However, in this case, that seems like an unnecessary optimization, since the difference between doing the work once and doing it a few times is unlikely to matter.

I will update the PR.

@cmeeren
Copy link
Contributor Author

cmeeren commented Feb 13, 2024

Done.

@glen-84 glen-84 changed the title Make OptionTypeConverte thread-safe Make OptionTypeConverter thread-safe Feb 13, 2024
@michaelstaib michaelstaib merged commit 3297b2d into ChilliCream:main Feb 13, 2024
97 of 98 checks passed
@cmeeren cmeeren deleted the patch-1 branch February 14, 2024 05:58
@PascalSenn PascalSenn mentioned this pull request Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants