Document that CancellationToken in Stream.ReadAsync() is advisory

[binki]

At https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.readasync?view=netframework-4.7.1 I read:

If the operation is canceled before it completes, the returned task contains the Canceled value for the Status property.

As described in #19867, this is not true in actual practice. Especially because NetworkStream.ReadAsync() ignores the CancellationToken altogether.

Thus, I think the documentation for Stream.ReadAsync() should be changed to indicate that the parameter is advisory. For code accepting a Stream, there is no way (without reflection which should not be required of consumers—and, even with reflection, one may be consuming a stream which wraps NetworkStream) to tell if the CancellationToken will be ignored or not. So such code always needs to be written in such a way as to support streams with uncancellable read (and write?) operations.

[stephentoub]

In general, cancellation is best-effort / cooperative. Regardless of the API, there are no guarantees that requesting cancellation will actually force that it happens.

You can submit PRs to improve the .NET docs at https://github.com/dotnet/docs.

[binki]

If cancellation is really that optional, then I guess I need to start writing a lot more complex/unreadable code to safely consume things without hanging indefinitely :-/. Especially if it isn’t guaranteed that the things which are cancellable on the framework will also be cancellable in Core or on particular platforms.

If the documentation could identify for me which places I can expect it to actually work, that’d be very helpful.

[Clockwork-Muse]

@binki -
Part of the problem is that cancellation is something inherently susceptible to race conditions.
What happens if you cancel the token, but the runtime is already in the process of throwing an exception? Or already returning you results? Or it only checks once a minute, and did just before the token was signaled.
And that assumes the cancellation would happen "immediately" in the first place. There might be resources to clean up. If I'm cancelling a database operation, I probably have to call a different function on the database - what happens if the database connection goes down at that point? Or even if the network is just slow, and the cancellation will be propagated in a minute or two...

For the NetworkStream case, you could set the timeout properties (and translate the returned socket exception during timeouts)

[jnm2]

If cancellation is really that optional, then I guess I need to start writing a lot more complex/unreadable code to safely consume things without hanging indefinitely

Cancellation is advisory because advisory cancellation is the only in-process cancellation that is remotely safe. The other option is Thread.Abort. If you have a real-world need to go there, that is so desperately unsafe (state-corruption side effects) that you should move the work out-of-proc and kill the process instead.

[binki]

@Clockwork-Muse What I care about with cancellations is that I can rely on DoSomethingAsync(ct) completing (either faulting, succeeding, or being cancelled) in a somewhat timely manner if ct were ever cancelled if the docs suggest I can rely on that behavior. The fact that cancellation is complicated to implement or consume properly might be unrelated(?). Also, the docs for NetworkStream.ReadTimeout state the timeout is ignored for asynchronous reads.

If the documentation were worded to warn the reader that it is advisory for a particular method—and, in inheritance scenarios, any subclass treating it as advisory caused the warning to be “bubbled up” to the super class’s documentation—then the documentation would actually be reflecting reality. Then I would know that the following can hang forever and is thus insufficient:

var n = await stream.ReadAsync(buf, 0, buf.Length, ct);

and that instead I must do something like the following if I want my method to enter the cancelled state in a timely manner after ct is cancelled and I promise to myself never to touch stream after ct is cancelled:

var readTask = stream.ReadAsync(buf, 0, buf.Length, ct);
await await Task.WhenAny(
    readTask,
    Task.Delay(-1, ct));
var n = await readTask;

As the docs for Stream.ReadAsync() are right now, I do not think this is obvious to the casual reader. That is why I filed this bug.

Reading Docs

The docs for the parameter:

cancellationToken CancellationToken
The token to monitor for cancellation requests. The default value is None.

The remarks:

If the operation is canceled before it completes, the returned task contains the Canceled value for the Status property.

Together, I am convinced that this documentation would be interpreted as: “The passed cancellationToken will be monitored. When the cancellationToken is cancelled, the returned Task will complete in a timely manner as Cancelled unless if a partial read was already in progress in which case the Task will be resolved with a value in a timely manner.” It doesn’t seem obvious that the returned Task is allowed to stay incomplete forever after the cancellation token is cancelled.

I guess I feel bad to use this argument. But, I don’t get “the Task may hang forever even if I cancel the cancellationToken” out of the docs when reading them. In fact, I read the opposite. Doesn’t this mean the documentation is unclear and could be improved?

[JonHanna]

https://msdn.microsoft.com/en-us/library/system.threading.cancellationtoken(v=vs.110).aspx

A CancellationToken enables cooperative cancellation between threads, thread pool work items, or Task objects. [emphasis mine]

[binki]

@JonHanna “cooperative” simply means that it is an alternative to forceful external cancellation which would be like calling Thread.Abort() or killing a process. CancellationToken provides a way to run code immediately in response to a request for cancellation which you can use to call the underlying APIs for the underlying asynchronous operation to cancel it. If an operation does not support cancellation, it either should not accept a CancellationToken in the first place (e.g., TextReader.ReadLineAsync() does not because it is not an atomic operation) or should have it at least explicitly documented that it is treated as advisory.

[JonHanna]

No cooperative cancellation is immediate, except through extreme luck. You're always waiting on the next check point. Just how frequent those check points should be isn't the easiest thing to judge, but if something is waiting on async I/O to respond there isn't anywhere to do that check.

The page you link to is about running code (not aborting it) in response to cancellation, and it isn't immediate either, unless the token is already cancelled.

[binki]

@JonHanna

The page you link to is about running code (not aborting it) in response to cancellation, and it isn't immediate either, unless the token is already cancelled.

Handlers registered with CancellationToken.Cancel() run synchronously before CancellationTokenSource.Cancel() returns except for handlers which have captured a SynchronizationContext, in which case the captured context schedules the cancellation handler. Depending on how the scheduling for the completion of the Task is being scheduled, it is possible that the Task can be marked cancelled before CancellationTokenSource.Cancel() returns without blocking. That seems consistent with “immediate” to me.

Of course, not everything can be synchronously cancelled. However, most implementations of things which support cancellation respond to the cancellation in a timely fashion instead of leaving the task pending forever.

[dinobu]

@JonHanna

The page you link to is about running code (not aborting it) in response to cancellation, and it isn't immediate either, unless the token is already cancelled.

Handlers registered with CancellationToken.Cancel() run synchronously before CancellationTokenSource.Cancel() returns except for handlers which have captured a SynchronizationContext, in which case the captured context schedules the cancellation handler. Depending on how the scheduling for the completion of the Task is being scheduled, it is possible that the Task can be marked cancelled before CancellationTokenSource.Cancel() returns without blocking. That seems consistent with “immediate” to me.

Of course, not everything can be synchronously cancelled. However, most implementations of things which support cancellation respond to the cancellation in a timely fashion instead of leaving the task pending forever.

Running into same issue and dumb documentation almost 2 years later :(

[wfurt]

Even if cancelation is best effort, with cancelation plumbed through sockets now, should cancelation on NetworkStream.ReadAsync() just work now in 3.0 @stephentoub?

[stephentoub]

should cancelation on NetworkStream.ReadAsync() just work now in 3.0 @stephentoub?

Yes

[dquist]

I would also like to chime in on this and say that the SerialPort class is another instance where ReadAsync used with a cancellation token will hang indefinitely. @binki has already done a phenomenal jobs describing why the docs are confusing in this regard, so I'll leave my comment at that.

[wfurt]

documentation is public and you can submit PR with improvement to https://github.com/dotnet/docs @dquist

Fact that SerialPort IO is not cancelable can be also problem with implementation. cc @krwq

[krwq]

@dquist can you file separate issue for SerialPort and give example code/test? Which architecture and OS are you running on? (please add in the issue, let's not offtopic here)

[dquist]

I'm running on core 3.0 preview 9 on Windows 1903.

I'd be happy to create a separate issue, however after browsing the source it seems this is the correct behavior.

SerialPort.Windows.cs inherits from Stream.cs which implements ReadAsync as

public virtual Task<int> ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
{
    // If cancellation was requested, bail early with an already completed task.
    // Otherwise, return a task that represents the Begin/End methods.
    return cancellationToken.IsCancellationRequested
                ? Task.FromCanceled<int>(cancellationToken)
                : BeginEndReadAsync(buffer, offset, count);
}

Notice that the cancellation token is only considered if cancellation has already been requested, otherwise it's just a wrapper for BeginEndReadAsync that takes no token parameter.

Given that SerialStream.Windows.cs does not override this behavior, it would seem that the cancellation token is in fact ignored when reading from the serial port base stream.

Interestingly, the Unix implementation of SerialStream does implement its own ReadAsync method that passes the cancellation token onto a SerialStreamIORequest.

I have not tested the ReadAsync behavior on Linux, however I wonder if the Windows implementation could also provide a similar method.

Edit: According to dotnet/corefx#33027 which added the Linux implementation, the first detail bullet says 'supports cancellation'. So this seems that the Linux implementation supports cancellation whereas the Windows implementation does not.

Edit2: Sorry, just saw your comment about not derailing this issue, but I would argue this is still relevant since it has to do with the various implementations of Stream.ReadAsync of which NetworkStream was only one example.

[krwq]

@dquist I've created the issue about this.

I think I kinda always assumed cancellation is a best effort and might not work immediately or be ignored by implementation but I think it makes sense to clarify the docs about it.

[davidsh]

I think I kinda always assumed cancellation is a best effort and might not work immediately or be ignored by implementation

In general, .NET cancellation via the CancellationToken semantics are always cooperative and best-effort. Implementations monitor the cancel token and try to cancel at the appropriate place, if possible. But cancellation is never guaranteed.

If there are .NET APIs that completely, 100%, ignore the cancellation token then we should document that. But in general, we don't need to document every API we have regarding how .NET cancellation semantics work.

cc: @stephentoub

[dquist]

I understand that the cancellation is best effort, but it's especially confusing when there are multiple Stream implementations that handle cancellation differently, not to mention two platform-specific implementations of SerialStream that differ wildly on how cancellation is handled.

This makes me sad from an API consumer standpoint since it makes it nearly impossible to build a consistent API using SerialPort.BaseStream as an abstraction because I have no idea if my token will be respected or swallowed.

I appreciate you taking a look at this, for now I will have to assume that any Stream implementation will not respect the cancellation token and rely on another mechanism.

[krwq]

@dquist note that most of the time the Read/WriteTimeout should be used for cancellation since this covers most typical scenarios and I believe they work correctly on both platforms - let's move SerialPort specific conversation to the other issue so it doesn't get lost.

[sipsorcery]

To add yet another voice of agreement to this issue. The ReadAsync code seems pointless other than to confuse callers. Since the only time the token gets checked is when the ReadAsync method is called why bother? The caller can check the token themselves before they make the call no need to pass it as a parameter.

Adding the cancellation token parameter to ReadAsync at best saves the caller a single if statement and comes at the cost of lots of head scratching and confusion.

[krwq]

@sipsorcery I agree Async methods are something we'd like to eventually have and I don't think anyone disagrees about having them. The problem is that adding support for async methods is not always super trivial and comparing to other issues we have async methods might not be exactly as high priority... Note, that Async methods are directly on the Stream so we cannot remove them even when we don't support them and I think it's still better to provide any implementation which works for positive cases (no cancellation) than throw

[stephentoub]

At this point, I don't see anything actionable in this issue, so I'm going to close it. If there are places where the docs could be more informative, we'd welcome PRs to https://github.com/dotnet/dotnet-api-docs to do so. And if there are places where the implementation isn't as prompt in responding to cancellation as it could/should be, we'd also welcome PRs to https://github.com/dotnet/runtime to improve things. Thanks.