views:

55

answers:

2

I have a library with an API much like this:

public class Library : IDisposable
{
    public Library(Action callback);
    public string Query();
    public void Dispose();
}

After I have instantiated Library, at any time and on any thread it might invoke the callback I have passed to it. That callback needs to call Query to do useful work. The library will only stop calling my callback once disposed, but if a callback attempts to call Query after the main thread has called Dispose, bad stuff will happen.

I do want to allow the callbacks to run on multiple threads simultaneously. That's okay. But we need to be sure that no callbacks can be running when we call Dispose. I thought a ReaderWriterLockSlim might be appropriate - you need the write-lock to call Dispose, and the callbacks need read-locks to call Query. The problem here is that ReaderWriterLockSlim is IDisposable, and I don't think it will ever be safe to dispose it - I can never know that there is not a callback in flight that simply hasn't gotten to the point of acquiring the read-lock yet.

What should I do? It looks like ReaderWriterLock isn't IDisposable, but it's own documentation says you should use ReaderWriterLockSlim instead. I could try to do something equivalent with just the "lock" keyword, but that sounds wasteful and easy to screw up.

PS - Feel free to say that the library API is not good if you think that's the case. I would personally prefer that it guaranteed that Dispose would block until all callbacks had completed.

+2  A: 

This sounds like something you can wrap with your own API which makes the guarantee from the final paragraph.

Essentially, each callback should atomically register that it's running, and check whether it's still okay to run - and then either quit immediately (equivalent to never being called) or do its stuff and deregister that it's running.

Your Dispose method just needs to block until it finds a time when nothing's running, atomically checking whether anything's running and invalidating if not.

I can imagine this being done reasonably simply using a simple lock, monitor, Wait/Pulse approach. Your API wrapper would wrap any callback it's given inside another callback which does all this, so you only need to put the logic in one place.

Do you see what I mean? I don't have time to implement it for you right now, but I can elaborate on the ideas if you like.

Jon Skeet
An interesting idea, but it does sound like there's ample opportunity for race conditions there.
Jim Mischel
@Jim: The whole point would be to put all the complexity of *avoiding* race conditions into one place. You write this one wrapper carefully, and the rest of the code can be rather simpler.
Jon Skeet
@Jon: Yes, I understand the concept. Would have to be done very carefully, though.
Jim Mischel
I think I understand, although I have a feeling I still might manage to screw it up. It looks like I might not have to. I think I can persuade the library owner to give it a slightly safer API.
Weeble
@Weeble: The library owner would probably have to write it carefully too. Why not try to implement it, then post the implementation here for scrutiny?
Jon Skeet
+2  A: 

This is a rather difficult problem to solve if you had to attempt it on your own. The pattern I am going to describe here uses the CountdownEvent class as the fundamental synchronization mechanism. It is available in .NET 4.0 or as part of the Reactive Extensions download for .NET 3.5. This class is an ideal candidate for problems in this genre because:

  • it can maintain a count.
  • it can wait for that count to reach zero.

Let me describe the pattern. I have created a CallbackInvoker class which contains only two operations.

  • It can invoke the callback synchronously using the Invoke operation.
  • It can receive a stop signal and wait for an acknowledgement using the FinishAndWait operation.

The Library class creates and uses an instance of CallbackInvoker. Anytime Library needs to invoke the callback it should do so by calling the Invoke method. When it is time to dispose the class just call FinishAndWait from the Dispose method. This works because the moment the CountdownEvent is signaled from FinishAndWait it locks out the TryAddCount in an atomic fashion. That is why the wait handle is initialed to a count of 1.

public class Library : IDisposable
{
    private CallbackInvoker m_CallbackInvoker;

    public Library(Action callback)
    {
        m_CallbackInvoker = new CallbackInvoker(callback);
    }

    public void Dispose()
    {
        m_CallbackInvoker.FinishAndWait();
    }

    private void DoSomethingThatInvokesCallback()
    {
        m_CallbackInvoker.Invoke();
    }

    private class CallbackInvoker
    {
        private Action m_Callback;
        private CountdownEvent m_Pending = new CountdownEvent(1);

        public CallbackInvoker(Action callback)
        {
            m_Callback = callback;
        }

        public bool Invoke()
        {
            bool acquired = false;
            try
            {
              acquired = m_Pending.TryAddCount();
              if (acquired)
              {
                  if (m_Callback != null)
                  {
                      m_Callback();
                  }
              }
            }
            finally
            {
              if (acquired) m_Pending.Signal();
            }
            return acquired;
        }

        public void FinishAndWait()
        {
            m_Pending.Signal();
            m_Pending.Wait();
        }

    }
}
Brian Gideon