0

What can cause issues when thread is aborted?

I need to use Thread.Abort() in my code because the thread runs complex code that have a lot of loops, objects and conditions.

I know that Thread.Abort() can lead to deadlock when using Monitor, also it can prevent resources from being released, but I can handle these problems.

I use IDisposable/using pattern or catch ThreadAbortException to guarantee that all resources are released and asynchronous operations are stopped.

The application seems to works fine now. But, since the code is pretty complex, I'm not sure if there could be some rare cases when aborting the thread can lead to memory leaks or unhandled exceptions.

Is there any .net classes (e.g. FileStream, Dictionary) that can cause problems if thread is aborted when their code are executed? Or some other issues that I should be aware of?

781F
  • 11
  • I'm not so sure that your thread really dies when you ask for it. – Marco Salerno Apr 24 '19 at 09:01
  • 1
    You should better try cooperative cancellation https://learn.microsoft.com/en-us/dotnet/standard/threading/cancellation-in-managed-threads – CSDev Apr 24 '19 at 09:05
  • I do know about `CancellationToken` but it's gonna be a lot of work to refactor the code to cancel it quick enough – 781F Apr 24 '19 at 09:32
  • @MarcoSalerno it dies, I've checked for it. – 781F Apr 24 '19 at 09:35
  • Also `Thread.Abort()` can leave the .NET run-time in an invalid state - it means that you cannot trust any of the remaining threads to behave properly after aborting a thread. It's for that reason that aborting threads **should only ever be done when crashing out of an app**. – Enigmativity Apr 26 '19 at 09:14
  • @Enigmativity thanks for that note, can you provide more info? – 781F Apr 27 '19 at 10:45
  • @781F - Have a read of http://blog.peterritchie.com/Thread.Abort-is-a-Sign-of-a-Poorly-Designed-Program/ – Enigmativity Apr 28 '19 at 07:06
  • That article is wrong about lock statements - it's out of date. `lock` now uses a new overload of `Monitor.Enter` which is safe to a thread abort happening. Other types of lock, however, are not. – canton7 May 03 '19 at 07:13

1 Answers1

2

The problem with Thread.Abort is that your ThreadAbortException can be thrown between any two instructions (almost).

If you take some very simple code like:

public void M()
{
    using (CreateThing())
    {
    }
}

public IDisposable CreateThing() => null;

And look at the generated C# and IL:

public void M()
{
    IDisposable disposable = CreateThing();
    try
    {
    }
    finally
    {
        if (disposable != null)
        {
            disposable.Dispose();
        }
    }
}

You can see that there's a couple of instructions between CreateThing being called, and it entering the try block. There's a small window of opportunity where, if Thread.Abort is called right then, your object will not be disposed.

So using IDisposable and using does not guarantee that your resources get freed in the face of Thread.Abort.

There is a very good reason why Thread.Abort was removed from .NET Standard, and why you should use CancellationToken instead.

You should use CancellationToken, and your code should check whether it's been cancelled (using CancellationToken.ThrowIfCancellationRequested()) at suitable, safe points.

As an aside, lock statements use an overload of Monitor.Enter which returns a boolean saying whether the lock was actually acquired, and:

lock (lockObject)
{
}

compiles to:

bool lockTaken = false;
try
{
    Monitor.Enter(lockObject, ref lockTaken);
}
finally
{
    if (lockTaken)
        Monitor.Exit(lockObject);
}

to avoid exactly this problem.

However, you don't get this luxury when using any other synchronization methods - only lock - and so you can easily deadlock.

canton7
  • 37,633
  • 3
  • 64
  • 77
  • > So using `IDisposable` and `using` does not guarantee that your resources get freed in the face of `Thread.Abort`. But if I do it in finalizer and using GC.Collect, then I should be fine? – 781F Apr 24 '19 at 09:25
  • It depends what your `Dispose` method is doing. Not everything can be done by the finalizer - if your `Dispose` method unsubscribes from an event / observable, or releases a lock, or does anything which *isn't* freeing an unmanaged resource, then you're still in trouble. – canton7 Apr 24 '19 at 09:29
  • (Also, you need to do a combination of a couple of `GC.Collects` and a `WaitForPendingFinalizers` to wait for finalizers to run, and the full GCs it needs are *expensive*) – canton7 Apr 24 '19 at 09:30
  • Basically you're playing with fire by relying on Thread.Abort. Various people have discovered this for themselves over the years. You may think you're clever enough to solve all the problems, but because this issue is inherently racy, there are problems which will *only* occur once a year in production, and you will not be able to debug or fix them. – canton7 Apr 24 '19 at 09:38
  • You should have a read of [Eric Lippert's reply to a question about Thread.Abort()](https://stackoverflow.com/a/1560567/106159): `Thread.Abort is at best indicative of bad design, possibly unreliable, and extremely dangerous. It should be avoided at all costs` – Matthew Watson Apr 24 '19 at 09:47
  • @MatthewWatson That is also a very good and valid read, and probably deserves its own answer here (or perhaps this should be closed as a duplicate) – canton7 Apr 24 '19 at 09:48