Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have a code that is accessed by multiple threads.

If by chance it has a problem, all accessing threads will wait for his recovery. By default rule, the first thread that fails, will to recover and all others will be waiting. When the process is complete.. everyone leaves the method.

I have so far:

        [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");
                }

            }
        }

for my tests .. Works well! But I believe that necessary adjustments and it can be implemented differently.

share|improve this question
    
It's not quite clear what do you want us to review... AutoRecover() looks like just a stub, right? Are you asking to review the test method? –  almaz Dec 14 '12 at 13:55
    
I would like to better ways, other ways of dealing with concurrency. –  J. Lennon Dec 14 '12 at 15:39

1 Answer 1

up vote 1 down vote accepted

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();
    }
}
share|improve this answer
    
Is a good approach. I thought of using semaphores, it may also be a good option. –  J. Lennon Jan 3 '13 at 12:03

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.