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;
}
}
}
}
Many Cores 
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());
}
}
LikeLike
It looks like you identify the issue. But your fix has impacts, coul you identify them and comment for thebenefits of all
LikeLike
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.
LikeLike