Skip to content

Making the cache dictionary thread safe#153

Merged
Keboo merged 2 commits intomoq:masterfrom
Keboo:makeThreadSafe
Oct 3, 2022
Merged

Making the cache dictionary thread safe#153
Keboo merged 2 commits intomoq:masterfrom
Keboo:makeThreadSafe

Conversation

@Keboo
Copy link
Collaborator

@Keboo Keboo commented Oct 3, 2022

Fixes #149

@Keboo Keboo requested a review from adamhewitt627 October 3, 2022 05:12
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 enabled auto-merge (squash) October 3, 2022 19:33
@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