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.

So I wrote an asynchronous version of AutoResetEvent:

public sealed class AutoResetEventAsync {
    private readonly Task<bool> falseResult = Task.FromResult(false);
    private readonly ConcurrentQueue<Handler> handlers = new ConcurrentQueue<Handler>();
    private readonly Task<bool> trueResult = Task.FromResult(true);
    private int isSet;

    public AutoResetEventAsync(bool initialState) {
        this.isSet = initialState ? 1 : 0;
    }

    public void Set() {
        this.isSet = 1;

        Handler handler;

        // Notify first alive handler
        while (this.handlers.TryDequeue(out handler))
            if (handler.TryUnsubscribe()) {
                handler.Invoke();
                break;
            }
    }

    public Task<bool> WaitAsync() {
        return this.WaitAsync(CancellationToken.None);
    }

    public Task<bool> WaitAsync(TimeSpan timeout) {
        if (timeout == Timeout.InfiniteTimeSpan)
            return this.WaitAsync();

        var tokenSource = new CancellationTokenSource(timeout);
        return this.WaitAsync(tokenSource.Token);
    }

    public Task<bool> WaitAsync(CancellationToken cancellationToken) {
        // Short path
        if (this.TryReset())
            return this.trueResult;

        if (cancellationToken.IsCancellationRequested)
            return this.falseResult;

        // Wait for a signal
        var completionSource = new TaskCompletionSource<bool>(false);

        var handler = new Handler(() => {
            if (this.TryReset())
                completionSource.TrySetResult(true);
        });

        // Subscribe
        this.handlers.Enqueue(handler);

        if (this.TryReset()) {
            // Unsubscribe
            handler.TryUnsubscribe();
            return this.trueResult;
        }

        cancellationToken.Register(() => {
            // Try to unsubscribe, if success then the handler wasn't invoked so we could set false result,
            // else the handler was invoked and the result is already set. 
            if (handler.TryUnsubscribe())
                completionSource.TrySetResult(false);
        });

        return completionSource.Task;
    }

    private bool TryReset() {
        return Interlocked.CompareExchange(ref this.isSet, 0, 1) == 1;
    }

    private sealed class Handler {
        private readonly Action handler;
        private int isAlive = 1;

        public Handler(Action handler) {
            this.handler = handler;
        }

        public void Invoke() {
            this.handler();
        }

        public bool TryUnsubscribe() {
            return Interlocked.CompareExchange(ref this.isAlive, 0, 1) == 1;
        }
    }
}

In my application it works well.

The question is: am I missing some concurrency issue?

I tested concurrent invocation of WaitAsync method, so, maybe you could suggest a better test scenario?

share|improve this question
    
The code is missing any public documentation, so one is left to wonder what exactly is the intended behavior. For example: it WaitAsync() supposed to be FIFO? –  svick Mar 6 '13 at 22:11
add comment

1 Answer

up vote 4 down vote accepted

Your code does have race conditions, so it won't work correctly. I think your main problem is that you make some “destructive” operation (e.g. dequeue a handler from the queue) without knowing that the followup operation will actually succeed and not handling the eventual failure in any way.

For example, the following sequence of events could happen:

  1. The class is created in unset state.
  2. WaitAsync() is called, creating a Handler, adding it to the queue and returning a Task.
  3. Set() is called, setting isSet to 1.
  4. The handler from step 2 is dequeued.
  5. WaitAsync() is called from another thread. Since isSet is 1, it sets isSet back to 0 and immediately returns a completed Task.
  6. Back on the previous thread, TryUnsubscribe() is called in the handler from step 4 and succeeds.
  7. The handler is Invoke()d, but TryReset() fails.
  8. Set() now returns.

After this, the Task from step 2 is now orphaned, it will never be completed, because its handler is not in the queue anymore.


I suggest you don't write code like this by yourself, but instead use code written by others with more experience in concurrent programming. Specifically, you could use Stephen Toub's AsyncAutoResetEvent or AsyncAutoResetEvent from Stephen Cleary's AsyncEx library.

share|improve this answer
    
Thank you for that. I saw AsyncAutoResetEvent that you mention, but it uses lock. I want to make a lock-less implementation of AutoResetEvent to get an experience in concurrent programming. I also spot another concurrency issue in WaitAsync method where it ignores the result of TryUnsubscribe method. –  SHSE Mar 7 '13 at 7:17
    
@SHSE Yeah, I wouldn't be surprised if there were other issues in your code. I just answered with the first significant race I discovered. –  svick Mar 7 '13 at 14:40
    
@SHSE Also, I'm not sure trying to be lock-less is worth it here, since this code is not likely to be in a tight loop. Of course, if you're doing this to learn, that might not matter much. –  svick Mar 7 '13 at 14:44
    
I ended up with this code, it isn't lock-less but it hasn't any instance wide locks. –  SHSE Mar 7 '13 at 18:07
add comment

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.