Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

stephentoub
Copy link
Member

Port #27464 to release/3.1

Description

When StreamReader.ReadAsync is called and the reader's buffer is initially empty, it calls a helper method that in turn interacts with the underlying stream. That helper method wasn't defined to take a CancellationToken, so ReadAsync doesn't pass along the one supplied by the caller. That means for this first read on the underlying stream, the supplied CancellationToken is ignored.

Customer Impact

Cancellation ends up being unreliable.

Regression?

No. It's been like this since the overload was added in .NET Core 2.1.

Testing

New tests were added. Previously tests were only validating the case where the token has cancellation requested prior to the invocation, which triggers a dedicated path that handles this correctly.

Risk

Low. It's just passing along a CancellationToken.

cc: @danmosemsft

…net#27464)

ReadAsyncInternal is correctly passing its token into the two call sites where it delegates directly to the underlying stream, but in one place it calls through ReadBufferAsync, which is currently not defined to take a token.  Fix that, and pass the token through.
@stephentoub stephentoub added the Servicing-consider Issue for next servicing release review label Oct 28, 2019
@stephentoub stephentoub added this to the 3.1 milestone Oct 28, 2019
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-approved Approved for servicing release Servicing-consider Issue for next servicing release review labels Oct 29, 2019
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmoseley
Copy link
Member

Rerunning one leg which never ran.

@danmoseley danmoseley merged commit c3fddaa into dotnet:release/3.1 Oct 31, 2019
@stephentoub stephentoub deleted the port27464 branch November 23, 2019 18:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants