Questions you should ask before using a callback / delegate

I just got burned by using callbacks in a multi-threaded app. I’ve rewritten the part to avoid callbacks, but for my good-deed-of-the-day, I wanted to issue a word of warning.

Before you can safely use a callback / delegate / virtual function, particularly in multi-threaded code, you should answer the following questions:

  1. Do I hold locks while invoking the callback?  If “yes”, then how do you know the callback won’t take a lock that causes a deadlock?  If “no”, is this because you’re unsafely releasing a lock just to invoke the callback? In the “no” case, you’re now preventing certain operations from being atomic because you need to toggle a lock to invoke a callback.

  2. What is the callback allowed to block on? Can the callback take locks? If “no”, then how do you enforce this? If yes, how do you avoid deadlocking? Can the callback make a cross-thread call? If “yes”, how do you ensure that thread is free? If “no”, again, how do you enforce it? Simply calling an STA object may invoke a marshaller that goes across threads.  Can it make system calls?

  3. What is the callback allowed to do? What if the callback throws an exception? What if the callback calls a major function that changes the world out from underneath you (eg, attempting to delete your object)? Can the callback invoke back into your API? Are there reentrancy or layering issues? Could there be infinite recursion here (you invoke the callback, callback invokes you, repeat)?

  4. Does your function make any assumptions across the callback for which the callback could break underneath you? For example, do you assume that a global variable is constant, when the callback could change the value of that global?

  5. Is the callback to 3rd-party code outside of my immediate library?  That greatly amplifies the unknowns. Now issues of enforcement really come into play. If a 3rd-party callback changes the world underneath you and then returns and your code crashes, then the exception’s callstack is pointing at you and the 3rd-party code has already made a get-away. Even if you think you control all the code, in a large project, that’s still a lot of room for surprises.  

  6. Are you in a restricted context? For example, are you on the UI thread? Do you really want to be calling unpredictable code on your UI thread? That’s a nice recipe to get your app to hang daily.

The questions really could be endless. If you’re code-reviewing somebody else’s code that uses callbacks, these are great questions to ask.

And it applies to managed code too:
Also beware, Virtual functions are basically callbacks. In C#, delegates and events are callbacks, but with a pretty syntax than C++ function pointers.

Callbacks and ICorDebug
The regular reader may realize that ICorDebug uses callbacks to invoke managed debug events (via ICorDebugManagedCallback), and we definitely hit some of these issues. For example, what if you try to Detach() during the middle of a callback? Calling Continue() within a callback vs. outside a callback exercise very different code-paths. These corner cases caused us extra grief when implementing ICorDebug. Since ICD doesn’t have nice short answers here, I’ll blog about that separately.

Comments (5)

  1. Peter Ritchie says:

    Great post.

    There’s a thread on MSDN Forums that exemplifies another problem using asynchronous delegates when implicitly multithreaded:  The gist is (one that you didn’t mention) don’t call Event-based Asynchronous Pattern asynchronous methods from an asynchronous delegate.

  2. Great post. Regarding the locking issues, I would argue that never taking a lock on a publicly accessible object is a good thing. If you need to share a lock amongst objects then you can expose a property / method to get / take the lock. That way you can control access to the lock.

    I have long thought that .NET repeated a mistake made by platforms before it by allowing arbitrary locking on any object. IMHO, locks should only be permitted on a special purpose Lock struct / class. This would encourage encapsulation of locks and would have the added benefit of reducing the overhead associated with each object on the managed heap.

  3. David Levine says:

    These are all very valuable questions that devs should ask when designing and implementing a callback of any type (delegates are one type).

    I try to never make assumptions about what the callee will do in the callback. I assume that some piece of code somewhere will abuse the callback and leave me to try to pick up the pieces, so I try to program as defensively as possible. Many of my "golden rules" are similar to yours. This means…

    1. All callbacks must be rentrant or handle reentrancy issues. For example, I always assume that a call to initiate another operation can occur within a completion routine.

    2. I never call the callback with locks held. I’ve not run into a case where I needed to hold a lock – I’ve redesigned a few components to remove the need.

    3. Always assume the callee will cause an error condition….so wrap your callback in a try-catch handler with suitable and well-defined error semantics (ignore/swallow, propagate, rethrow, crash, etc).

    Associated questions are: if one callback raises an error how will that affect other subscribers to an event or callback? Does it cancel it? Abort it?

    Are errors saved and reported elsewhere? Discarded? Logged? Traced?

    4. Make no assumptions about thread state on return. Priorities may have changed, security contexts may be different, etc. This is similar to your example about static variables – it is state shared across instances. If global state is altered how should the caller react?

    5. Make no assumptions about how long it will take the callee to complete and return, or what they will do in the callback routine. Also consider using asynchronous versus synchronous callback mechanisms – this may avoid running into some deadlock problems, especially when calling back into a UI thread.

    6. I consider calling back using the original caller’s synchronization context – this helps in situations where some callbacks occur on non-UI threads while others occur on UI threads. If its a UI thread I generally prefer to use async callbacks.

    If you must constrain what the callback is allowed to do then it should be part of the intellisense documentation and well documented elsewhere so that devs that are otherwise unfamiliar with your component will have as good a chance as possible of understanding and programming against those constraints.

    It would be nice if somehow the component itself could enforce the constraints that callbacks must adhere to, but I think we are lightyears away from such an environment.

  4. David – those are great defensive rules.

Skip to main content