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

Making the cache dictionary thread safe #153

Merged
merged 2 commits into from
Oct 3, 2022
Merged

Conversation

Keboo
Copy link
Collaborator

@Keboo Keboo commented Oct 3, 2022

Fixes #149

tkellogg
tkellogg previously approved these changes Oct 3, 2022
Copy link
Collaborator

@tkellogg tkellogg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a thread-safe dictionary is a start. I also don't see any simple read-then-write patterns. It seems to only (1) read everything xor (2) write one or two items, not both.

This PR attains the basic assumptions of thread safety, as far as I can tell. However, if a caller tries to setup multiple tests simultaneously, this will lead to a race condition. This seems intuitive to the user, so I don't think we should worry about it.

One concern - I haven't checked the docs, but is ToDictionary threadsafe? I'm pretty sure it's an extension method, but maybe the enumerator returned by the threadsafe dictionary is also threadsafe. One can hope but I'd rather verify. Code comments help (e.g. explicitly state why you think an operation is threadsafe, it helps future code readers be comfortable with it).

@Keboo
Copy link
Collaborator Author

Keboo commented Oct 3, 2022

You are correct on all counts. ToDictionary is just the LINQ extension method (which by itself is not thread-safe), however the NonBlocking.ConcurrentDictionary does return a snapshot enumerator. I also pushed up a change adding a comment to say the same.

I do also agree that we should not try to handle the case where a single AutoMocker instance is shared across multiple tests. IMO this leads to tests that are difficult to maintain with mock objects containing multiple setups. However, with the case that was originally brought up, was passing off the AutoMocker instance as an IServiceProvider into the code being tested. In those cases, it does make some sense to at lease ensure that we don't throw exceptions, even if timing issues will still be a problem. For example it is still very much possible to have two threads resolve the and create the same type (or mocks) with only one getting cached and the other being discarded.
IMO this is acceptable. If a caller wants all access to the AutoMocker instance synchronized, a simple wrapper with a lock statement will do the trick. If, however, we try to handle the above timing cases to ensure that it always is in a consistent state, there will be a performance penalty for the locking.

adamhewitt627
adamhewitt627 previously approved these changes Oct 3, 2022
@Keboo Keboo merged commit 9efffb7 into moq:master Oct 3, 2022
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.

GetMock<T> is not thread-safe
3 participants