Simplifying and optimize a concurrent access

Question

I have code that is accessed by multiple threads. If by any chance it has a problem, all accessing threads will wait for recovery. By default, the first thread that fails, will have to recover and all others will have to wait. When the process is complete, everyone leaves the method.

[TestMethod]
public void RecoveryConcurrentTest()
{
    Task task = Task.Factory.StartNew(() =>
        {
            ManualResetEvent r1 = new ManualResetEvent(false);
            var t1 = new Thread((obj) =>
            {
                AutoRecover();
                r1.Set();
            });

            ManualResetEvent r2 = new ManualResetEvent(false);
            var t2 = new Thread((obj) =>
            {
                AutoRecover();
                r2.Set();
            });

            ManualResetEvent r3 = new ManualResetEvent(false);
            var t3 = new Thread((obj) =>
            {
                AutoRecover();
                r3.Set();
            });

            ManualResetEvent r4 = new ManualResetEvent(false);
            var t4 = new Thread((obj) =>
            {
                AutoRecover();
                r4.Set();
            });
            t1.Start();
            t2.Start();
            t3.Start();
            t4.Start();

            WaitHandle.WaitAll(new WaitHandle[] { r1, r2, r3, r4 });
        });
    task.Wait();
}

private Object _syncRoot = new Object();
private int count;
private bool handled;
public void AutoRecover()
{
    Trace.WriteLine("thread: " + Thread.CurrentThread.ManagedThreadId);
    Interlocked.Increment(ref count);
    Trace.WriteLine("count + 1 = " + count); // not work properly

    lock (_syncRoot)
    {
        Trace.WriteLine("thread: " + Thread.CurrentThread.ManagedThreadId);
        if (handled)
        {
            Trace.WriteLine("handled is true");
        }
        else
        {
            Trace.WriteLine("handled is false");
            Trace.WriteLine("PROCESSING....");
            Thread.Sleep(10000);
            Trace.WriteLine("PROCESSED....");
            handled = true;
            Trace.WriteLine("handled set true");
        }

        Interlocked.Decrement(ref count);
        Trace.WriteLine("count - 1 = " + count);
        if (count == 0)
        {
            handled = false;
            Trace.WriteLine("handled set false");
        }
    }
}

It works well for my tests, but I believe that there are necessary adjustments and that it can be implemented differently.

Answer 1

There is no sense in creating a task and waiting on it straight away. The following code is almost identical to your test method (almost - because tasks are created on a thread pool here):

[TestMethod]
public void RecoveryConcurrentTest()
{
    var tasks = Enumerable.Range(0, 10).Select(i => Task.Factory.StartNew(AutoRecover)).ToArray();

    Task.WaitAll(tasks);
}

Note that it doesn't actually test the fact that AutoRecover allows only one thread at a time, it just ensures that nothing fails when multiple threads call this method. There is no way in current design to detect it.

What I would suggest is to expose the fact of long process inside of AutoRecover by returning a Task that any thread can wait on:

[TestMethod]
public void RecoveryConcurrentTest()
{
    var tasks = Enumerable.Range(0, 10).Select(i => AutoRecoverAsync()).ToArray();

    Task.WaitAll(tasks);
}

private volatile Task _recoveryTask;
private readonly object _syncRoot = new object();

private async Task DoActualRecover()
{
    Trace.WriteLine("PROCESSING....");
    await Task.Delay(10000);
    Trace.WriteLine("PROCESSED....");
    _recoveryTask = null;
}

public Task AutoRecoverAsync()
{
    var recoveryTask = _recoveryTask;
    if (recoveryTask != null)
        return recoveryTask;

    lock (_syncRoot)
    {
        recoveryTask = _recoveryTask;
        if (recoveryTask != null)
            return recoveryTask;

        return _recoveryTask = DoActualRecover();
    }
}