Concurrency Warning#288
Conversation
# Conflicts: # source/Nevermore/Advanced/DeadlockAwareLock.cs
YuKitsune
changed the title
| public interface ITransactionConcurrencyHandler : IDisposable | ||
| { | ||
| IDisposable Lock(); | ||
|
|
||
| Task<IDisposable> LockAsync(CancellationToken cancellationToken); | ||
| } |
There was a problem hiding this comment.
We decided to replace the DeadlockAwareLock with this abstraction as a way for us to enable logging without locking. It's a bit much, but felt cleaner than introducing a bunch of additional checks into a single class.
Open to alternatives.
There was a problem hiding this comment.
Here's a spike of what this would look like if we had a single type: #289
| return configuration.SupportConcurrentExecution | ||
| ? new ThreadSafeEnumerable<TRecord>(Execute, DeadlockAwareLock) | ||
| // Use the thread safe variant if we know it's going to be used | ||
| return configuration.ConcurrencyMode is not ConcurrencyMode.NoLock |
There was a problem hiding this comment.
Suggestion: A clearer and more reliable way to make this decision could be
- Add a
bool IsThreadSafe { get; }property toTransactionConcurrencyHandler - The implementations can return true/false appropriately
return TransactionConcurrencyHandler.IsThreadSafe ? new ThreadSafeEnumerable<TRecord>...
If you wanted to go full-hog with this you could hide the creation of the ThreadSafeEnumerable inside the concurrency handler, and then do return TransactionConcurrencyHandler.WrapEnumerable(Execute()) all the time with zero if statements. That'd be the safest and "best" in terms of OOP, but I'm not sure I'd personally take it that far, as it introduces a new concern (Enumerables) into the ConcurrencyHandler. Maybe I'll change my mind in the morning.
There was a problem hiding this comment.
Underlying principle here: You've got one input - the ConcurrencyMode enum, but you make decisions about it in two places. If the decisions don't need to align, that's fine -- however in this case the choice to use ThreadSafeEnumerable, and the behaviour of the TransactionConcurrencyHandler are highly aligned. If you used a locking handler without the threadsafe enumerable (or vice versa) that would be a bug.
Technique: Reduce the decision-points (and thus, risk of bugs) by having one decision "flow through" to the next. If that all sounds like gibberish, hit me up for a zoom tomorrow 😄
There was a problem hiding this comment.
I'm keen to follow up here on zoom. Another concern here is that we still want to use the logging-only concurrency handler.
I think the cleanest way forward is to always return the ThreadSafeEnumerable, and give it what ever concurrency handler we've got. The only thing I don't like about that is the fact that we can return a ThreadSafeEnumerable with a concurrency handler that doesn't provide thread safety, but we need it for the logging anyway 😕
borland
left a comment
borland
left a comment
There was a problem hiding this comment.
I'm quite happy with the removal of DeadlockAwareLock, the choice of four implementations of concurrency handler, and the interface is nice. I would appreciate a quick zoom though, I'd like someone to show me all the places you found where the deadlockawarelock wasn't working properly.
Some comments about the implementation details in ReadTransaction, nothing obviously bad, but it'd be nice to have a chat about them before I approve.
Note: I'm not blocking; if someone else is happy to approve before I get there and you want to merge, fair game
borland
left a comment
borland
left a comment
There was a problem hiding this comment.
All looking good, approved!
4 participants
[sc-82478]
DeadlockAwareLock(Most calls to it were usingLock()andLockAsync()which bypassed the deadlock checks)DeadlockAwareLockwithITransactionConcurrencyHandlerSupportConcurrentExecutionoption withConcurrencyModeoptionThreadSafeEnumerables withEnumerableWithConcurrencyHandlingfor clarity