When implementing time-constrained methods, should I abort the worker thread or let it run its course?

Question

I'm currently writing a web services based front-end to an existing application. To do that, I'm using the WCF LOB Adapter SDK, which allows one to create custom WCF bindings that expose external data and operations as web services.

The SDK provides a few interfaces to implement, and some of their methods are time-constrained: the implementation is expected to complete its work within a specified timespan or throw a TimeoutException.

Investigations led me to the question "Implement C# Generic Timeout", which wisely advises to use a worker thread. Armed with that knowledge, I can write:

public MetadataRetrievalNode[] Browse(string nodeId, int childStartIndex,
    int maxChildNodes, TimeSpan timeout)
{
    Func<MetadataRetrievalNode[]> work = () => {
        // Return computed metadata...
    };

    IAsyncResult result = work.BeginInvoke(null, null);
    if (result.AsyncWaitHandle.WaitOne(timeout)) {
        return work.EndInvoke(result);
    } else {
        throw new TimeoutException();
    }
}

However, the consensus is not clear about what to do with the worker thread if it times out. One can just forget about it, like the code above does, or one can abort it:

public MetadataRetrievalNode[] Browse(string nodeId, int childStartIndex,
    int maxChildNodes, TimeSpan timeout)
{
    Thread workerThread = null;
    Func<MetadataRetrievalNode[]> work = () => {
        workerThread = Thread.CurrentThread;
        // Return computed metadata...
    };

    IAsyncResult result = work.BeginInvoke(null, null);
    if (result.AsyncWaitHandle.WaitOne(timeout)) {
        return work.EndInvoke(result);
    } else {
        workerThread.Abort();
        throw new TimeoutException();
    }
}

Now, aborting a thread is widely considered as wrong. It breaks work in progress, leaks resources, messes with locking and does not even guarantee the thread will actually stop running. That said, HttpResponse.Redirect() aborts a thread every time it's called, and IIS seems to be perfectly happy with that. Maybe it's prepared to deal with it somehow. My external application probably isn't.

On the other hand, if I let the worker thread run its course, apart from the resource contention increase (less available threads in the pool), wouldn't memory be leaked anyway, because work.EndInvoke() never gets called? More specifically, wouldn't the MetadataRetrievalNode[] array returned by work remain around forever?

Is this only a matter of choosing the lesser of two evils, or is there a way not to abort the worker thread and still reclaim the memory used by BeginInvoke()?

Answer 1

Well, first off Thread.Abort is not nearly as bad as it used it to be. There were several improvements made to the CLR in 2.0 that fixed several of the major issues with aborting threads. It is still bad, mind you, so avoiding it is the best course of action. If you must resort to aborting threads then at the very least you should consider tearing down the application domain from where the abort originated. That is going to be incredibly invasive in most scenarios and would not resolve the possible corruption of unmanaged resources.

Aside from that, aborting in this case is going to have other implications. The most important being that you are attempting to abort a ThreadPool thread. I am really not sure what the end result of that would be and it could be different depending on which version of the framework is in play.

The best course of action is to have your Func<MetadataRetrievalNode[]> delegate poll a variable at safe points to see if it should terminate execution on its own.

public MetadataRetrievalNode[] Browse(string nodeId, int childStartIndex, int maxChildNodes, TimeSpan timeout)
{
    bool terminate = false;

    Func<MetadataRetrievalNode[]> work = 
      () => 
      {
        // Do some work.

        Thread.MemoryBarrier(); // Ensure a fresh read of the terminate variable.
        if (terminate) throw new InvalidOperationException();

        // Do some work.

        Thread.MemoryBarrier(); // Ensure a fresh read of the terminate variable.
        if (terminate) throw new InvalidOperationException();

        // Return computed metadata...
      };

    IAsyncResult result = work.BeginInvoke(null, null);
    terminate = !result.AsyncWaitHandle.WaitOne(timeout);
    return work.EndInvoke(result); // This blocks until the delegate completes.
}

The tricky part is how to deal with blocking calls inside your delegate. Obviously, you cannot check the terminate flag if the delegate is in the middle of a blocking call. But, assuming the blocking call is initiated from one of the canned BCL waiting mechansisms (WaitHandle.WaitOne, Monitor.Wait, etc.) then you could use Thread.Interrupt to "poke" it and that should immediately unblock it.