Modifying object inside of lock with the same lock

I have a job object made up of a collection of work items. Each job has it's own WatcherClass associated with it that checks the database every so often to see if it needs to cancel execution. It could be cancelled at any iteration in the workflow. IF it is cancelled, any threads running from the foreach block will propagate the cancellation and exit gracefully.

Is there any problem in my watcher code that could create a deadlock? I am trying to only allow one thread to process on the timer callback by using Timer.Change(Timeout.Infinite, Timeout.Infinite), but does the fact that I am changing WatcherClass.Job inside the lock statement defeat the lock's purpose (since I wrapped the same get/set for _Job in the same lock object)? The code appears to be working fine, but I know that is no indication of anything really.

The code in the main thread looks similiar to this:

using (WatcherClass watcher = new WatcherClass())
{
    watcher.CancelTokenSource = new CancellationTokenSource();
    watcher.Start();
    foreach (SomeJob job in worksflow.Jobs)
    { 
        watcher.Job = job;
        //Do some stuff async
        //Do some more stuff async
    }

}

public class WatcherClass : IDisposable
{
    private System.Threading.Timer _WatcherTimer;
    private readonly object locker = new object();
    private bool _Disposed = false;
    private SomeJob _Job;

    public SomeJob Job
    {
        get
        {
            lock (locker)
            {
                return _Job;
            }
        }
        set
        {
            lock (locker)
            {
                _Job= value;
            }
        }
    }

    public System.Threading.Task.CancellationTokenSource 
        CancelToken { get; set; }

    public WatcherClass()
    {
        _WatcherTimer = ne开发者_Go百科w Timer(new TimerCallback(DoCheck), null, 
            Timeout.Infinite, Timeout.Infinite);
    }

    public void Start()
    {
        _WatcherTimer.Change(30000, Timeout.Infinite);
    }

    public void DoCheck(object state)
    {

        lock (locker)
        {

            if (_Disposed || this.CancelToken.IsCancellationRequested)
                return;

            _WatcherTimer.Change(Timeout.Infinite, Timeout.Infinite);

            //Check database to see if task is cancelled
            if (cancelled)
            {
                this.CancelToken.Cancel();
                _Job.CancelResult = CancelResult.CanceledByUser;
                _Job.SomeOtherProperty = true;
            }
            else
            {
                //Safe to continue
                _WatcherTimer.Change(30000, Timeout.Infinite);
            }
        }

    }

    public void Dispose(bool disposing)
    {
        lock (locker)
        {
            if (disposing)
            {
                if (_WatcherTimer != null)
                    _WatcherTimer.Dispose();

            }
            _Disposed = true;
        }
    }
}

The lock you aquire around the Task property and in the DoCheck function only protects access to the internal _task field of the WatcherClass. In DoCheck, you are also modifying properties of the _task object itself. The lock does not prevent anyone else from also modifying the task object's fields at the same time from other threads.

If in your application the task object is only manipulated by DoCheck, then you're probably ok. If the task object may be manipulated by code other than DoCheck, then you may have a problem.

Also keep in mind that every additional lock you create is an additional opportunity for deadlock. Multiple locks can be deadlock-free if they are always acquired in a specific order. If the code flow allows for lock A to be acquired before lock B in some situations, or lock B before lock A in other situations, then you have a serious deadlock risk. (Thread 1 locks A, tries to lock B while thread 2 locks B and tries to lock A => deadlock)

In your WatcherClass case, if you are going to have multiple watcherclass instances each with their own locks, be careful not to make external calls (or fire events) that could end up trying to acquire locks in other watcherclass instances. That's an AB / BA deadlock waiting to happen.