A couple of days ago, one of our team members saw in the log that we are getting index out of bound exceptions in some of the code of the a piece of code I wrote (part of the EventBroker). Taking a look he saw the following:
       private void EventSender(object state)
{
var id = (Guid) state;
try
{
sagas[id].Dispatch();
}
catch (Exception e)
{

            Logger.DebugFormat("EventSender: Exeption Details={0}",e);
rwl.TryRLock();
var isClosed = true;
if (sagas.ContainsKey(id))
isClosed = sagas[id].IsClosed;
rwl.ExitReadLock();

if (!isClosed)
HandleSagaFault(id);

}
}

It is well known that one of the guidelines for exception throwing is "Do not use exceptions for normal or expected errors, or for normal flow of control".So,naturally, he raised an eyebrow and asked me why don't I check that the id exist before I try to call the Dispatch method.
e.g. something like:
            try
{

rwl.TryRLock();
var isClosed = true;
if (sagas.ContainsKey(id))

sagas[id].Dispatch();
rwl.ExitReadLock();
}

Well, as it happens the EventSender method runs on a thread pool thread (something you could have guessed from the object State parameter). It's role is to get the saga to dissipate the event. i.e. the Dispatch method on the Saga object handles actually sending   events to event's subscribers.And while it does its work in parallel it also waits for all the events to arrive before returning (collecting any communications exceptions etc.) What this means is that the Dispatch method takes awhile to return.

Ahh, there's the rub - Contrary to the guideline mentioned above, it is actually more worthwhile to pay the performance penalty of throwing an exception rather than pay the more severe penalty of holding a lock for a long time. The lock affects all the threads running whereas the exception only affects the isolated thread.

Indeed - on another MSDN page there a better version of the guideline "Do not use exceptions for normal flow of control, if possible. Except for system failures and operations with potential race conditions"

The lesson here is (again) is that we need to think before blindly following guidelines.Guidelines are, well er, guidelines not commandments

PS
if you release the lock before calling Dispatch you don't gain anything, since it is multi-threaded code and the id can still be collected between the check and the call itself.

PPS
in case you are wondering TryRLock() is an extension method which try to obtain a lock with a given (short) timeout and throw (instead of deadlock) if the timeout is reached.


 
Comments are closed.