Make AddTypeHandlerImpl in SqlMapper thread safe #2107
Conversation
|
Hey @mgravell I don't think a clone is necessary. I can run this review if confirmed to not be necessary. I see this is old code from 2015. There is comment |
| var newCopy = clone ? new Dictionary<Type, ITypeHandler>(snapshot) : snapshot; | ||
| lock (typeHandlers) | ||
| { | ||
| if (typeHandlers.TryGetValue(type, out var oldValue) && handler == oldValue) return; // nothing to do |
There was a problem hiding this comment.
Can you do this as a ConcurrentDictionary instead of locking?
|
The existing code perhaps has risk of lost updates, but this API is not expected to be used aggressively on-the-fly, so in reality: this shouldn't be a problem. If we're concerned, then some simple synchronization in those few methods should be fine. The proposed changes, however, look to be more unsafe - in that it allows competing threads doing routine query work (not adding handlers) to attempt to read the dictionary while it is being mutated (only the write is guarded by the lock). This can break in much worse ways. The old code deliberately avoided this by never allowing the "live" dictionary to be mutated. Potentially this can be mitigated using things like concurrentdictionary, but: I wonder if this would thing can be much simpler by just locking on a separate sync token for the duration of any admin operations, but maintaining the snapshot/mutate/swap approach. |
|
Regarding the "impl" method; damn, that method was never intended to be public. Separately, I'll deal with that (in a different PR). Just pretend that public API doesn't exist. |
|
@mgravell Right, this PR code currently isn't ideal. I can run this PR to get it into a good shape. Or if @mobilebilly is okay with it, I can just do my own PR tidying this section up. Even the existing "clone operation" can run afoul of issues during the iteration for the clone. I was thinking about setting it as a |
|
I've proposed #2129, which I think does everything needed in a safer way, with fewer moving parts. Thoughts? |
|
dammit appveyor, stop messing with pgsql! |
|
Closing out with preference to #2129 |
As discussed in issue #1815, the current implementation of SqlMapper.AddTypeHandlerImpl is not thread-safe. When typeHandlers is cloned during concurrent access to this method, a race condition occurs because each thread maintains its own copy of typeHandlers. This can result in handlers added by some threads being lost. This fix addresses the issue to ensure thread safety.
Additionally, we propose to obsolete below overload version of AddTypeHandlerImpl:
Also suggest making all overloads of AddTypeHandlerImpl non-public, as their method names indicate that they are intended for internal implementation, but that could be handled separately in the future.