Reactive: CanCancelMoveNext fails due to bug in ToAsyncEnumerable

[jnm2]

/cc @onovotny @shiftkey @NickCraver

Conversation started here: https://twitter.com/onovotny/status/755052889232506880

To summarize, the test has always passed because the blocking MoveNext was getting canceled while it was still scheduled via Task.Run, before it actually had a change to run on a thread.

Removing Where and Select from the test helps reduce the problem. They caused enough of a delay that the task actually ran before cancelling and the test failed, but a more deterministic and scoped way to test is to use a second ManualResetEvent to guarantee that the task runs. This avoids the race timing issue. (See https://github.com/jnm2/Rx.NET/commit/97522da57f2429039a9e83a8a9190a52935a243b)

What's lacking in ToAsyncEnumerable is the ability to cancel once the blocking code runs. The cancellation token is given to Task.Run but that only governs threadpool scheduling and has no effect once the code is running. True cancellation is only possible by cancellation-aware code, but MoveNext is not cancellation-aware so there is no way to prematurely end the task. (Barring aborting the thread, but that is worst-practice.)

So, given that the thread is not going to end and the task is not going to be completed no matter what we do with the cancellation token, we have only one option. ToAsyncEnumerable can't return the Task.Run task, because we can't cause it to go into a cancelled state. We must return a TaskCompletionSource task from ToAsyncEnumerable.

I suppose one other option is to consider the non-cancelability as a feature rather than a bug. Even though your async code continues as though it had cancelled, there is still a background thread running the blocking code. I'd be interested to hear everyone's thoughts on this.

Also, keep in mind that I've only laid eyes on Ix or Rx for the first time an hour ago. Hopefully this is in the spirit of the existing paradigm.

[jnm2]

Implemented the fix on both master and ix-optimizations. (ix-optimizations still has two unrelated failing tests.)

[jnm2]

So before I start a pull request, I know of two things that I want to have reviewed:

1. Is this the correct paradigm, abandoning a working thread and carrying on? Or is it better to not allow a cancel since the work cannot be cancelled? That would force you to pay attention to your architecture.
2. Does the CancellationTokenRegistration here need to be disposed? I assume GC is good enough since I think it's only holding onto tcs (though I'm not an expert at this). If continuations are registered off the TCS could this become problematic?

[NickCraver]

Thanks @jnm2, I've just gotten a bit to look at this. I think the main (very common) incorrect assumption is that the cancellation token has any effect once the task is running. IMO, how a cancellation token works in user code is not really well conveyed in general, almost every dev I meet (relatively new to async) thinks it _just works_ because the framework methods have code to check it they're unaware of. But since we can't fix that...

1. I'm don't think there's a winning answer here. Do you want to stop mid-way in all cases? What if it's a mutable operation? That'd be a mess to cleanup if not in a transaction, etc. I'm not sure it's a good idea to go down that path. For read-only, absolutely: cancel away. But only where that's guaranteed.
2. I think this should be left for the GC. You could definitely break downstream user code you can and will break things. There are some awesome optimization discussions around bits like this re: Kestrel from a few months back. Some resulted in far cheaper CancellationTokens in .NET Core though.

FWIW, the only way I see to _really_ fix this with the current setup would be to somehow expose the CancellationToken from the IAsyncEnumerable, but I see: an obvious breaking interface change and few people actually using this to check in their loops. The ones who _really_ need it could, though.

[clairernovotny]

Thanks gents for the extra set of eyes and analysis!

I think the main thing here was a faulty assumption/poor docs on what the CancellationToken does when passed to Task.Run. Having it stop the scheduling of a thread makes far more sense than doing any kind of Thread.Abort, which we can all agree is inherently unsafe.

I don't think there's really any good way to "fix" this issue either. If you have code that blocks/deadlocks/ignores cancellation tokens, then there's really nothing we can do. In this case, just because that code is hidden behind an iterator doesn't make it less broken.

FWIW, there are other issues with ToEnumerable and ToAsyncEnumerable: #201 and #202 come to mind. Overall, I think it's best to not use them at all. If anything, maybe add an [Obsolete] to them to encourage people to avoid them.

So that's the root cause here. Now on to what we can fix. I think we can fix the test itself in two ways:

1. What @jnm2 did initially, to show the blockage. I'd recommend reducing the wait time from 5s to 500ms or some such. This test should test for and show that it will block, that we know about it, and there's nothing we can do.
2. Write a different test that covers the original scenario -- that a CancellationToken works when passed into the MoveNext method. We should be able to write a test that confirms this behavior without deadlocks.

[jnm2]

Re [Obsolete]: I think ToAsyncEnumerable is fine as long as there is an expectation of no background threading. It would be non-cancellable and wrap using Task.FromResult. There still is a need for an interface adapter from IEnumerable to IAsyncEnumerable, right? Given the heavy usage in the tests I'd say so.

[clairernovotny]

@jnm2 I would recommend carrying on the design consideration of ToAsyncEnumerable over in #202. Let's keep this thread focused on the bug in the test.

[clairernovotny]

@jnm2 can you please submit a PR of your test fixes to master? are you then also up for writing a new unit test that actually tests the behavior of the cancellation token in the MoveNext method?

[jnm2]

@onovotny Sure. To make CanCancelToAsyncEnumerableMoveNext pass, we have three options. I need to know which you want:

1. Not test canceling ToAsyncEnumerable at all because it's inherently uncancellable
2. Use my ToAsyncEnumerable which simulates cancellation and probably encourages worst practice
3. Use a synchronous wrapper implementation of ToAsyncEnumerable. We would have to get rid of the evt MRE and the test would pass because await MoveNext(ct) would block and finish before we cancel. All this would do is show that ToAsyncEnumerable returns an IAsyncEnumerable that doesn't deadlock and with which the cancellation API can be used without special consideration. Not sure if it's worth writing for that?

The ultimate choice between 2 and 3 would depend on what you want to do with ToAsyncEnumerable. For now, are you asking for option 2? Option 1 isn't bad either.

In addition, I understand that the test wasn't originally intended to test ToAsyncEnumerable but rather cancellation, so you want a CanCancelMoveNext test that tests cancellation on a natively-async non-ToAsyncEnumerable enumerable, correct?

[clairernovotny]

@jnm2 For making it pass, I'd say that we "make it pass" by changing the test criteria -- that is, use task.Wait(500) or some such, to make it wait only 500 ms before throwing and resetting the MRE's. The test should validate that we do actually throw, thereby proving and documenting the issue.

For actually making a CanCancelMoveNext, you got it. Let's get a better test for that.

[jnm2]

@onovotny Ah, you want the test to assert the presence of the ToAsyncEnumerable issue. When the issue is resolved, the test will begin to fail. Am I understanding correctly?

[clairernovotny]

@jnm2 Indeed....and lets just add comments to the test to let future people know what to expect (and perhaps a link back to this thread).

[jnm2]

@onovotny Do I need to duplicate on ix-optimizations?

Also I'll put https://github.com/jnm2/Rx.NET/commit/fd7038ae44dd5cf38ae71257e1af752bed93087b in its own feature branch in my fork for future reference.

[clairernovotny]

@jnm2 please don't duplicate ix-optimizations for this fix, lets aim this at master. I'll deal with getting that into the branch.

[jnm2]

@NickCraver On a separate subject I'm curious about the downstream breaking issue you mentioned with CancellationTokenRegistration. I usually dispose them. Can you point me towards more info?

[jnm2]

@onovotny Since I'm using a second MRE, no wait is necessary. I'm going to do 0 ms.

I'm also wondering if the original test is meant to test whether .Select and .Where pass through cancellation, in which case I should roll my own IAsyncEnumerable implementation, or if I'm supposed to be testing some library implementation.

[clairernovotny]

@jnm2 I'd leave the original method as close as possible with using Select and Where and just create your own IAsyncEnumerator that's "testable" and can confirm that the token was received in its MoveNext method