Single lookups into concurrent dictionary #2231
Conversation
| lock (_targetInterceptors) | ||
| { | ||
| _targetInterceptors.TryGetValue(session, out var interceptors); | ||
|
|
||
| if (interceptors == null) | ||
| { | ||
| interceptors = new List<TargetInterceptor>(); | ||
| _targetInterceptors.TryAdd(session, interceptors); | ||
| } | ||
|
|
||
| interceptors.Add(interceptor); | ||
| } | ||
| var interceptors = _targetInterceptors.GetOrAdd(session, static _ => new()); | ||
| interceptors.Add(interceptor); |
There was a problem hiding this comment.
@kblok Do you recall if the lock in AddTargetInterceptor intended to lock both the usage of _targetInterceptors and adding an element to interceptors?
A colleague of mine pointed out that I might have introduced a race-condition here
If two threads get the same interceptors out of _targetInterceptors and concurrently calls the thread-unsafe List<T>.Add.
On the other hand the other usages of interceptors are also not locked
RemoveTargetInterceptorremoves an item from the listOnAttachedToTargetenumerates the list
There was a problem hiding this comment.
A colleague of mine pointed out that I might have introduced a race condition here
Is that a theory or a suspicion? We are talking about multiple threads, but I don't see how those events coming from the browser could be racy.
But yeah. The goal of the lock was to make those two actions thread-safe.
We could restore the lock. We could also turn the list into a ConcurrentSet.
On the other hand the other usages of interceptors are also not locked
- RemoveTargetInterceptor removes an item from the list
Again, I don't see how we can get a race in RemoveTargetInterceptor
OnAttachedToTarget enumerates the list
I can see the need for a lock there.
There was a problem hiding this comment.
A colleague of mine pointed out that I might have introduced a race condition here
Is that a theory or a suspicion? We are talking about multiple threads, but I don't see how those events coming from the browser could be racy.
Before this PR the non-thread safe interceptors.Add(interceptor); was inside a lock and now it isn't.
I didn't notice that slight change of semantics when I created the PR, that's why I'm bringing it to your attention.
We haven't experienced that any problems that I suspect stem from this PR, so for now it's a theoretical race-condition.
I assume that's what you meant by "theory or a suspicion"?
There was a problem hiding this comment.
@jnyrup yes. I was curious whether you did got a race condition or you found it could be a problem.
I think we could make it a ConcurrentSet just in case.
Found some more places where we can use built-in methods on
ConcurrentDictionary