C#: Invoke Event from Locked Block

Question

I have usually heard that it is a good idea to unlock any locks before calling event listeners to avoid deadlock. However, since the lock {} block is reentrant by the same thread in C#, is it OK to call events from a locked block or do I need to make a copy of the relevant state data and invoke the event outside the lock block?

If not, please give an example of when it would be a problem to call an event from within a lock {} block.

Thanks

Millikan

Answer 1

I don't recall ever having a need to raise an event from within a lock statement. But it's not hard to imagine things going badly.

When you raise an event, you defer execution to code that may not be your own. This is especially true if you are writing some library or framework, for example, that will be used by others. Inside the event handler, you have absolutely no control over what happens. The event handler could launch a brand new thread and wait for that thread to finish (i.e., Join()) before returning. If that new thread called some function that locked on the same variable as your lock, bingo. Deadlock.

But beyond that, the best practice is to minimize the amount of time you spend inside a lock in order to reduce the "choke point" aspect of the locking. If you raise an event inside the lock, all bets are off.

Mekong

The System.IO.Ports.SerialPort class is fairly simple to use - I had comms up and running very quickly from having never seen it before. I was missing some bytes because of the 'DiscardNull' property, and took a while to realize that the 'DataReceived' event may not be invoked even if there is some data waiting there. – Doubly

Great answer - why I always avoid calling event handlers or delegate methods inside a lock block; code which is beyond the control of the caller. – Koblenz

Answer 2

The problem isn't that the event handler might try to call into the lock you already have, the problem is that the event handler might try to acquire some other lock (possibly blocking and setting up for a deadlock), or that the event handler might start some long-running task like a database query (leaving your lock inaccessible to other threads until it finishes). The general rule is that you should hold a lock for as short a time as possible.

Mixing threading with event handlers that you don't control definitely opens you up for trouble. I'm currently having some trouble because I raised an event from the serial port's receiving thread. Some event handler code decided to block and wait until another message is received from the serial port. It's going to be a long wait, because you just blocked the one and only receiving thread! I can't even get mad at anyone because I wrote both pieces of code (a year apart so I had time to forget the details).

Glum

@Don Kirkby: Completely separate question. You mentioned "serial port." You're doing this in C#? Any good web sites, reference materials, books, etc. to look at? I'm going to need to tackle this in next few months. Any help would be appreciated. Sorry to hijack this thread. – Mekong

@Matt: It's not complicated. Just write a string or byte array and then read the response or register for the DataReceived event. I don't think you really need any more than the manual: msdn.microsoft.com/en-us/library/… – Glum

How does one protect against changes in state, during event handling, without a lock? Seems as simple as throwing an exception if an event handler attempts to acquire a lock or change state that it shouldn't instead of trying to re-lock. – Betoken

If the event handlers need to change the state, @Triynko, they should do so through accessor methods that acquire the lock. The sequence is: stabilize the state, release the lock, call event handlers, event handlers may call back to methods that acquire the lock again and change the state. – Glum

Answer 3

Yes it would not be considered good design to invoke an event from within a held lock. Mostly because there is a transition out of the locked section and if that were to throw an error or have a problem the lock would not necessarily be released.

In general you want to minimize the time you are in the lock to prevent holding the lock too long (causing delays) and also to reduce the chance that you will have code that can fail while the lock is held.

Arrow

If an event handler were called inside a lock and it threw an exception, the lock would be released. The lock keyword is basically shorthand notation for System.Threading.Monitor.Enter/Exit wrapped inside a try-finally block. That is, the Exit() method is called from the finally block, so the lock would be released. – Mekong

Actually that is not necessarily true. If the exception were propogated up to the caller who owns the lock yes, it would be released. If the exception occurred on another thread and left the caller in a deadlocked state (i.e. waiting on a return) then it would not. – Arrow

Answer 4

I would have left this as a comment on the accepted answer, but lacked the reputation.

In agreement with the other answers, yes, you should avoid raising events inside held locks. As the others pointed out, though, you don't know what other code inside the lock is doing.

Take for example ObservableCollection<T>; if you modify one inside a lock, it will raise CollectionChanged, and you can't stop that. In fact, to support BindingOperations.EnableCollectionSynchronization, it's required that the event is raised inside the lock!

Tletski