WaitHandle is closed when it shouldn't have

Question

This code works, for most of time, so I'm thinking of some race condition. Result class is immutable, but I don't think the issue is with that class.

public Result GetResult()
{
    using (var waitHandle = new ManualResetEvent(false))
    {
        Result result = null;

        var completedHandler = new WorkCompletedEventHandler((o, e) =>
        {
            result = e.Result;

            // somehow waitHandle is closed, thus exception occurs here
            waitHandle.Set();
        });
        try
        {
            this.worker.Completed += completedHandler;

            // starts working on separate thread
            // when done, this.worker invokes its Completed event
            this.worker.RunWork();

            waitHandle.WaitOne();

            return new WorkResult(result);
        }
        finally
        {
            this.worker.Completed -= completedHandler;
        }
    }
}

Edit: Apologies, I've missed a call to this.worker.RunWork() right before calling GetResult() method. This apparently resulted (sometimes) in doing same job twice, though I'm not sure why waitHandle got closed before waitHandle.Set(), despite having Completed event firing twice. This hasn't compromised the IO work at all (results were correct; after I've changed the code to manually close the waitHandle).

Therefore, Iridium's answer should be closest answer (if not the right one), even though the question wasn't complete.

Answer 1

There doesn't seem anything particularly problematic in the code you've given, which would suggest that there is perhaps something in the code you've not shown that's causing the problem. I'm assuming that the worker you're using is part of your codebase (rather than part of the .NET BCL like BackgroundWorker?) It may be worth posting the code for that, in case there is an issue there that's causing the problem.

If for example, the same worker is used repeatedly from multiple threads (or has a bug in which Completed can be raised more than once for the same piece of work), then if the worker uses the "usual" means for invoking an event handler, i.e.:

var handler = Completed;
if (handler != null)
{
    handler(...);
}

You could have an instance where var handler = Completed; is executed before the finally clause (and so before the completedHandler has been detached from the Completed event), but handler(...) is called after the using(...) block is exited (and so after the ManualResetEvent has been disposed). Your event handler will then be executed after waitHandle is disposed, and the exception you are seeing will be thrown.

Answer 2

There is no obvious reason why this would fail from the posted code. But we can't see a stack trace and we can't see the logic that gets the Completed event fired so there are few opportunities to debug this for you. Arbitrarily, if the event fires more than once then you'll certainly have this kind of race problem.

Vexing threading problems are hard to debug, threading races are problems that occur at microsecond scale. Trying to debug it can be enough to make the race disappear. Or it happens so infrequently that having any hope of catching the problem is too rare to justify an attempt.

Such problems often require logging to diagnose the race. Be sure to select a light-weight logging method, logging in itself can alter the timing enough to prevent the race from ever occurring.

Last but certainly not least: do note that there is no point in using a thread here. You get the exact same outcome by directly calling the code that's executed by whatever thread is started by RunWork(). Minus the overhead and the headaches.

Answer 3

If you get rid of the using your code will not throw an exception at the by you designated line... You have to find a decent place to dispose it, if you really need to.

public Result GetResult()
{
    var waitHandle = new ManualResetEvent(false);

        Result result = null;

        var completedHandler = new WorkCompletedEventHandler((o, e) =>
        {
            result = e.Result;

            // somehow waitHandle is closed, thus exception occurs here
            waitHandle.Set();
            waitHandle.Dispose();
        });
        try
        {
            this.worker.Completed += completedHandler;

            // starts working on separate thread
            // when done, this.worker invokes its Completed event
            this.worker.RunWork();

            waitHandle.WaitOne();

            return new WorkResult(result);
        }
        finally
        {
            this.worker.Completed -= completedHandler;
        }
}