Possible bug with AsyncManuelResetEvent.Reset

[niemyjski]

I have the following code, which I want to timeout without throwing an exception via a cancelation token via waitAsync (This code runs a ton and we don't want a trillion exceptions thrown). The problem is, we don't know if it timed out or passed through via the resetEvent so we want to reset the resetEvent (https://github.com/exceptionless/Foundatio/blob/async/src/Core/Lock/CacheLockProvider.cs#L37-L74)

            var resetEvent = new AsyncManualResetEvent(false);
            var sw = Stopwatch.StartNew();
            await Task.WhenAny(Task.Delay(100), resetEvent.WaitAsync()).ConfigureAwait(false);
            sw.Stop();
            Assert.InRange(sw.ElapsedMilliseconds, 100, 110);

            sw.Restart();
            resetEvent.Reset();
            sw.Stop();
            Assert.InRange(sw.ElapsedMilliseconds, 0, 5);

            sw.Reset();
            await Task.WhenAny(Task.Delay(100), resetEvent.WaitAsync()).ConfigureAwait(false);
            sw.Stop();
            Assert.InRange(sw.ElapsedMilliseconds, 100, 110);

The bug we see is that when we call resetEvent.Reset() the last Task.WhenAny() completes immediately. We think it should wait 100ms as it's been reset, but by looking at the code we see that the underlying tsc isn't reset.

[niemyjski]

Also, we were curious and hoping you'd be willing to do a code review / answer some questions with us on our oss foundatio project. We've been building this library for the whole community to benefit from things like locks and caching and we'd love your feedback.

[StephenCleary]

The problem is with the test: it's calling sw.Reset(); instead of sw.Restart();

However, I should caution you that using Task.WhenAny with asynchronous coordination primitives is extremely dangerous. Abandoning a task representing a wait it is not the same as cancelling a wait operation. In this case, it would work, but many coordination primitives work on a "queue" - so each call to WaitAsync (or whatever) will cause a queue entry to be created, and those tasks are completed as they're granted access. AMRE is one of the few coordination primitives without a queue.

Also, I strongly recommend against using timing for unit tests. All of the coordination primitives in AsyncEx are heavily tested, but none of the tests use timing for test verification. It takes a bit more work, but the tests are fully dependable if they don't use timings.

[StephenCleary]

Oh, and I would love to do a code review, but I just flat-out don't have time right now. Sorry...

[niemyjski]

Thanks for your input, I'll bring this up to the team and see if we can rework stuff. We just wanted to add the timing stuff to short circuit the tests if they are failing so it doesn't run for ever. Have you found a good way to do this?

[StephenCleary]

No, I just put an overall timeout on each test of a couple seconds. If they're over that, I assume it's a deadlock. It would be better to use Rx's TestScheduler, but I haven't taken the time to convert over to that yet.

[niemyjski]

Awesome! Thanks