Is this an appropriate place to invoke Thread.Abort()?

Question

I have some code that I borrowed from Steve Marx. The main block is used in an azure worker role thread to take out a lease on an azure blob. This provides a locking mechanism for synchronizing across multiple worker instances when you only want one instance to process a job at a time. However since you might have jobs that will take longer to complete than a blob lease timeout, a new thread is spawned to renew the blob lease every so often.

This renewal thread sleeps and renews on an infinite loop. When the main thread exits (via Dispose in the class' consumer), renewalThread.Abort() is invoked. This causes all kinds of ThreadAbortExceptions to be thrown in the worker role.

I'm wondering, is this a better way to handle this? What I don't like about it is that you can have several renewal threads that remain asleep after the consumer that spawned them has been disposed. Is there anything bad about the code below? If so, is there a better way? Or is Thread.Abort() appropriate here?

public class AutoRenewLease : IDisposable
{
    private readonly CloudBlockBlob _blob;
    public readonly string LeaseId;
    private Thread _renewalThread;
    private volatile bool _isRenewing = true;
    private bool _disposed;

    public bool HasLease { get { return LeaseId != null; } }

    public AutoRenewLease(CloudBlockBlob blob)
    {
        _blob = blob;

        // acquire lease
        LeaseId = blob.TryAcquireLease(TimeSpan.FromSeconds(60));
        if (!HasLease) return;

        // keep renewing lease
        _renewalThread = new Thread(() =>
        {
            try
            {
                while (_isRenewing)
                {
                    Thread.Sleep(TimeSpan.FromSeconds(40.0));
                    if (_isRenewing)
                        blob.RenewLease(AccessCondition
                            .GenerateLeaseCondition(LeaseId));
                }
            }
            catch { }
        });
        _renewalThread.Start();
    }

    ~AutoRenewLease()
    {
        Dispose(false);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (_disposed) return;
        if (disposing && _renewalThread != null)
        {
            //_renewalThread.Abort();
            _isRenewing = false;
            _blob.ReleaseLease(AccessCondition
                .GenerateLeaseCondition(LeaseId));
            _renewalThread = null;
        }
        _disposed = true;
    }
}

Update

Let's say you have an azure worker role deployed with 2 or more instances. Let's also say you have a job that both instances have the ability to process for you. During the worker roles Run method you might have something like this:

    public override void Run()
    {
        while (true)
        {
            foreach (var task in _workforce)
            {
                var job = task.Key;
                var workers = task.Value;
                foreach (var worker in workers)
                    worker.Perform((dynamic)job);
            }
            Thread.Sleep(1000);
        }
    }

Every second, the role will check to see if certain jobs are scheduled to run, and if they are, process them. However to avoid having both role instances process the same job, you first take out a lease on a blob. By doing that, the other instance cannot access the blob, so it is effectively blocked until the first instance is finished processing. (Note: taking out a new lease happens within the .Perform method above.)

Now, let's say a job can take anywhere from 1 to 100 seconds to complete. There is a built-in timeout on blob leases, so if you want to keep the other role blocked until the process is finished, you have to periodically renew that lease, to keep it form timing out. That is what the above class encapsulates -- automatically renewing a lease until you dispose of it as a consumer.

My question is mainly about the sleep timeout in the renewalThread. Say the job completed in 2 seconds. The renewalThread will gracefully exit (I think) but not for another 38 seconds. That is where the meat of uncertainty in my question lies. The original code invoked renewalThread.Abort(), which caused it to cease immediately. Is it better to do that, or let it sleep and exit gracefully at a later time? If you are heartbeating the role's Run method at once per second, you could have up to 40 of these renewal threads waiting to exit gracefully. If you have different jobs blocking on different blobs, that number gets multiplied by the number of blobs being leased. However if you do it with Thread.Abort(), you get just as many ThreadAbortExceptions sizzling on the stack.

Answer 1

As I understand it, you have a job that requires a lease on some object. That lease can expire, so you want something to continually renew the lease as long as the job is running.

You don't need a thread in a sleep loop. You need a timer. For example:

public class AutoRenewLease : IDisposable
{
    private readonly CloudBlockBlob _blob;
    public readonly string LeaseId;
    private System.Threading.Timer _renewalTimer;
    private volatile bool _isRenewing = true;
    private bool _disposed;

    public bool HasLease { get { return LeaseId != null; } }

    public AutoRenewLease(CloudBlockBlob blob)
    {
        _blob = blob;

        // acquire lease
        LeaseId = blob.TryAcquireLease(TimeSpan.FromSeconds(60));
        if (!HasLease) return;

        _renewalTimer = new System.Threading.Timer(x =>
        {
            if (_isRenewing)
            {
                blob.RenewLease(AccessCondition
                    .GenerateLeaseCondition(LeaseId));
            }
        }, null, TimeSpan.FromSeconds(40), TimeSpan.FromSeconds(40));


    ~AutoRenewLease()
    {
        Dispose(false);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (_disposed) return;
        if (disposing && _renewalTimer != null)
        {
            _isRenewing = false;
            _renewalTimer.Dispose();
            _blob.ReleaseLease(AccessCondition
                .GenerateLeaseCondition(LeaseId));
            _renewalTimer = null;
        }
        _disposed = true;
    }
}

There's no need to waste the resources that a thread uses just so that it can sleep most of the time. Using a timer eliminates polling and also eliminates the need for Thread.Abort.

Answer 2

Abort should be avoided whenever possible. There are some places you really need it, but for this scenario I think we can do it better without abort.

Make it simple with ManualResetEvent, This will stop your thread gracefully and immediately without the use of Abort.

private ManualResetEvent jobSignal = new ManualResetEvent(false);
public AutoRenewLease(CloudBlockBlob blob)
{
    _blob = blob;

    // acquire lease
    LeaseId = blob.TryAcquireLease(TimeSpan.FromSeconds(60));
    if (!HasLease) return;

    // keep renewing lease
    _renewalThread = new Thread(() =>
    {
        try
        {
            while (_isRenewing)
            {
                if(jobSignal.WaitOne(TimeSpan.FromSeconds(40.0)))
                {
                    //Disposed so stop working
                    jobSignal.Dispose();
                    jobSignal = null;
                    return;
                }
                if (_isRenewing)
                    blob.RenewLease(AccessCondition
                        .GenerateLeaseCondition(LeaseId));
            }
        }
        catch (Exception ex) {//atleast log it }
    });
    _renewalThread.Start();
}

protected virtual void Dispose(bool disposing)
{
    if (_disposed) return;
    if (disposing && _renewalThread != null)
    {
        jobSignal.Set();//Signal the thread to stop working
        _isRenewing = false;
        _blob.ReleaseLease(AccessCondition
            .GenerateLeaseCondition(LeaseId));
        _renewalThread = null;
    }
    _disposed = true;
}

Hope this helps.