reactive: CanCancelMoveNext fails due to bug in ToAsyncEnumerable

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.

About this issue

Original URL
State: closed
Created 8 years ago
Comments: 16 (16 by maintainers)

Commits related to this issue

Fixes #231 — committed to jnm2/Rx.NET by jnm2 8 years ago
Fixes #231 — committed to jnm2/Rx.NET by jnm2 8 years ago
Split CanCancel into two tests, fixes #231 — committed to jnm2/Rx.NET by jnm2 8 years ago

Most upvoted comments

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

clairernovotny on Jul 19, 2016

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…

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.
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.

NickCraver on Jul 19, 2016