Eagerly Disposing a ManualResetEvent

Question

I have a class which allows other threads to wait until it finishes an operation using a ManualResetEventSlim. (The operations are typically brief)

This class has no explicit lifetime, so there is no single place that I can easily close the event.
Instead, I want to close the event as soon as it's finished with—as soon as it's signalled, and after any waiting threads wake up.

For performance reasons, I'd prefer not to use a lock.

Is this code thread-safe, and can it be made faster?

volatile bool isCompleted;
volatile int waitingCount;
ManualResetEventSlim waiter = new ManualResetEventSlim();

//This method is called on any thread other than the one that calls OnCompleted
public void WaitForCompletion() {
    if (isCompleted)
        return;

    Interlocked.Increment(ref waitingCount);
    Thread.MemoryBarrier();
    if (!isCompleted)
        waiter.Wait();

    if (0 == Interlocked.Decrement(ref waitingCount)) {
        waiter.Dispose();
        waiter = null;
    }
    return;
}

//This method is called exactly once.
protected internal virtual void OnCompleted(string result) {
    Result = result;
    isCompleted = true;
    Thread.MemoryBarrier();
    if (waitingCount == 0) {
        waiter.Dispose();
        waiter = null;
    } else
        waiter.Set();
}

Answer 1

The biggest thing I see with your code is the setting of the waiter to null after calling Dispose. I have a large body of managed wrappers over unmanaged interfaces that I am in charge of and when I moved to .Net 4.0 this practice came back to bite me in some threading scenarios.

The MSDN information on ManualResetEventSlim.Dispose suggests that it is not threadsafe, however, looking over its actual implementation there is nothing dangerous about multiple invocations of Dispose from multiple threads. Additionally, implementations of IDisposable are supposed to be very tolerant of multiple invocations (as specified in their design guidance).

One idea I'd toyed with would reorder OnCompleted slightly to allow a reader if it subscribes shortly after it completes:

//This method is called exactly once.
protected internal virtual void OnCompleted(string result) {
    Result = result;
    isCompleted = true;

    waiter.Set();
    Thread.MemoryBarrier();
    if (waitingCount == 0) {
        waiter.Dispose();
    }
}

Answer 2

Worse, there are conditions under which it won't dispose of the waiter at all. If you call OnCompleted when waitingCount > 0, the isCompleted flag will get set to true, but the waiter won't be disposed. When something calls WaitForCompletion, it will see that isCompleted is true, and exit immediately. waiter.Dispose never gets called.

Why not use something like SpinLock, which uses the same kind of logic as ManualResetEventSlim? If your waits are typically very short, then the lock probably won't be contended and it's a huge win. If the wait is long, then the ManualResetEventSlim was going to pay the price of the kernel transition anyway.

Are you so certain that using a lock would be prohibitively expensive? There's "knowing," and then there's measuring . . .