Can You Spot the Deadlock 2?


Hello dear readers and coders

Welcome to the second ‘Can You Spot the Deadlock?’ trivia. I surely hope you had fun with the first one. Yeah, I know, it was kinda easy. So today, I raise the bar a bit and bring you one of my favorites: the transactional subscription/un subscription pattern.

The code compiles but it will not run, as I did not show the scaffolding code. Suffice to say that there is a notification mechanism that lives in its own thread(s) and some client code logic that rely on the main(starting) thread.

So:

  • What is the issue here?
  • Where does it occur?
  • Can you fix it ? if yes how, if now, why

Problem is now closed, please find the solution

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace SpotTheDeadLock2
{
///
/// Implements aynchronous notification
///
public class EventSource
{
    private EventHandler _eventHandler;
    ///
    /// notification event
    ///

    public event EventHandler EventOccured
    {
        add {
            // lock to secure subscription
            lock (this){
                _eventHandler += value;
            }
        }
        remove {
            // lock to secure unsubscription
            lock (this) {
                _eventHandler -= value;
            }
        }
    }
    // implementation of notification
    private void TriggerEvent() {
        lock (this) {
            if (_eventHandler != null) {
                _eventHandler(this, new EventArgs());
            }
        }
    }
}

///
/// client implementation
///
public class Client: IDisposable
{
    private EventSource _source;
    ///
    /// Simple constructor
    ///
    ///data source to subsribe to
    public Client(EventSource source) {
        _source = source;
        _source.EventOccured += OnEvent;
    }

    ///
    /// Method in charge of processing events
    ///
    private void OnEvent(object sender, EventArgs arguments) {
        // use lock as we do not have control of calling thread
       lock (this) {
           // do my job
           ...
       }
    }
    // unsubscribe on clean up
    public void Dispose() {
    // use lock to ensure no processing is in progress
        lock (this) {
            _source.EventOccured -= OnEvent;
        }
    }
}
}
Advertisements

5 thoughts on “Can You Spot the Deadlock 2?

  1. I really like your CYSDL: very funny 😉

    Ok; here, a deadlock may occur if an event is raised during the Dispose of one client instance. Indeed, imagine the following sequence:

    Thread1: Client.Dispose() -> lock(ClientLock)

    then

    Thread2: EventProducer.TriggerEvent() -> lock(EventProducerLock) -> Client.OnEvent (callback) -> blocked on the lock(ClientLock)

    then Thread1: continues with: _source.EventOccured -= OnEvent; -> EventProducer.EventOccured.Remove -> blocked on the lock(EventProducerLock)

    At the end of the day:
    – Thread1 has tried to acquire locks in the following order: (ClientLock) and then (EventProducerLock)
    – Thread 2 has tried to acquire locks in the order: (EventProducerLock) and then (ClientLock)

    which both led them in a deadlock situation. Sad Panda ;-(

    This lead us to recall one of the core multi-threading rule: “NEVER call a delegate after having taken a lock” (otherwise you will multiply your odds of a deadlock by a hundred factor)

    To fix this deadlock, changes the TriggerEvent() implementation like this:

    // Still naïve, but less dangerous implementation of notification
    private void TriggerEvent()
    {
    lock (this)
    {
    var copiedMulticastDelegate = _eventHandler;
    }

    if (copiedMulticastDelegate != null)
    {
    // calls the delegate safely
    copiedMulticastDelegate(this, new EventArgs());
    }
    }

    Like

  2. There is a similar case using autoreset event. I face it few years ago. As it is an event you can many handlers. But only one handler will be notify when the event is raised. Of course it is said in MSDN doc, but I still think that is not a good pattenr to provide an event interface “hoping” users will subscribe only one time.

    Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s