Synchronization mechanism for an observable object

.net, c#, multithreading, thread-synchronization

Solution

This sounds like quite the multi-threading pickle. It's quite challenging to work with recursion in this chain-of-events pattern, whilst still avoiding deadlocks. You might want to consider designing around the problem entirely.

For example, you could make the addition of an operand asynchronous to the raising of the event:

private readonly BlockingCollection<Operand> _additions
    = new BlockingCollection<Operand>();

public void AddNewOperand(Operand operand)
{
    _additions.Add(operand);
}

And then have the actual addition happen in a background thread:

private void ProcessAdditions()
{
    foreach(var operand in _additions.GetConsumingEnumerable())
    {
        _container.Lock.EnterWriteLock();
        _container.Operands.Add(operand);
        _container.Lock.ExitWriteLock();
    }
}

public void Initialize()
{
    var pump = new Thread(ProcessAdditions)
    {
        Name = "Operand Additions Pump"
    };
    pump.Start();
}

This separation sacrifices some consistency - code running after the add method won't actually know when the add has actually happened and maybe that's a problem for your code. If so, this could be re-written to subscribe to the observation and use a `Task` to signal when the add completes:

public Task AddNewOperandAsync(Operand operand)
{
    var tcs = new TaskCompletionSource<byte>();

    // Compose an event handler for the completion of this task
    NotifyCollectionChangedEventHandler onChanged = null;
    onChanged = (sender, e) =>
    {
        // Is this the event for the operand we have added?
        if (e.NewItems.Contains(operand))
        {
            // Complete the task.
            tcs.SetCompleted(0);

            // Remove the event-handler.
            _container.Operands.CollectionChanged -= onChanged;
        }
    }

    // Hook in the handler.
    _container.Operands.CollectionChanged += onChanged;

    // Perform the addition.
    _additions.Add(operand);

    // Return the task to be awaited.
    return tcs.Task;
}

The event-handler logic is raised on the background thread pumping the add messages, so there is no possibility of it blocking your foreground threads. If you await the add on the message-pump for the window, the synchronization context is smart enough to schedule the continuation on the message-pump thread as well.

Whether you go down the `Task` route or not, this strategy means that you can safely add more operands from an observable event without re-entering any locks.

Problem

Let's imagine we have to synchronize read/write access to shared resources. Multiple threads will access that resource both in read and writing (most of times for reading, sometimes for writing). Let's assume also that each write will always trigger a read operation (object is observable). For this example I'll imagine a class like this (forgive syntax and style, it's just for illustration purposes): ``` class Container { public ObservableCollection<Operand> Operands; public ObservableCollection<Result> Results; } ``` I'm tempted to use a `ReadWriterLockSlim` for this purpose moreover I'd put it at `Container` level (imagine object is not so simple and one read/write operation may involve multiple objects): ``` public ReadWriterLockSlim Lock; ``` Implementation of `Operand` and `Result` has no meaning for this example. Now let's imagine some code that observes `Operands` and will produce a result to put in `Results`: ``` void AddNewOperand(Operand operand) { try { _container.Lock.EnterWriteLock(); _container.Operands.Add(operand); } finally { _container.ExitReadLock(); } } ``` Our hypotetical observer will do something similar but to consume a new element and it'll lock with `EnterReadLock()` to get operands and then `EnterWriteLock()` to add result (let me omit code for this). This will produce an exception because of recursion but if I set `LockRecursionPolicy.SupportsRecursion` then I'll just open my code to dead-locks (from MSDN): By default, new instances of ReaderWriterLockSlim are created with the LockRecursionPolicy.NoRecursion flag and do not allow recursion. This default policy is recommended for all new development, because recursion introduces unnecessary complications and makes your code more prone to deadlocks. I repeat relevant part for clarity: Recursion [...] makes your code more prone to deadlocks. If I'm not wrong with `LockRecursionPolicy.SupportsRecursion` if from same thread I ask a, let's say, read lock then someone else asks for a write lock then I'll have a dead-lock then what MSDN says makes sense. Moreover recursion will degrade performance too in a measurable way (and it's not what I want if I'm using `ReadWriterLockSlim` instead of `ReadWriterLock` or `Monitor`). Question(s) Finally my questions are (please note I'm not searching for a discussion about general synchronization mechanisms, I would know what's wrong for this producer/observable/observer scenario): - What's better in this situation? To avoid `ReadWriterLockSlim` in favor of `Monitor` (even if in real world code reads will be much more than writes)? - Give up with such coarse synchronization? This may even yield better performance but it'll make code much more complicated (of course not in this example but in real world). - Should I just make notifications (from observed collection) asynchronous? - Something else I can't see? I know that there is not a best synchronization mechanism so tool we use must be right one for our case but I wonder if there are some best practice or I just ignore something very important between threads and observers (imagine to use Microsoft Reactive Extensions but question is general, not tied to that framework). Possible solutions? What I would try is to make events (somehow) deferred: 1st solution Each change won't fire any `CollectionChanged` event, it's kept in a queue. When provider (object that push data) has finished it'll manually force the queue to be flushed (raising each event in sequence). This may be done in another thread or even in the caller thread (but outside the lock). It may works but it'll make everything less "automatic" (each change notification must be manually triggered by producer itself, more code to write, more bugs all around). 2nd solution Another solution may be to provide a reference to our lock to the observable collection. If I wrap `ReadWriterLockSlim` in a custom object (useful to hide it in a easy to use `IDisposable` object) I may add a `ManualResetEvent` to notify that all locks has been released in this way collection itself may rise events (again in the same thread or in another thread). 3rd solution Another idea could be to just make events asynchronous. If event handler will need a lock then it'll be stopped to wait it's time frame. For this I worry about the big thread amount that may be used (especially if from thread pool). Honestly I don't know if any of these is applicable in real world application (personally - from users point of view - I prefer second one but it implies custom collection for everything and it makes collection aware of threading and I would avoid it, if possible). I wouldn't like to make code more complicated than necessary.

Original source