Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Sep 17, 2021

Backport of #59065 to release/6.0

/cc @stephentoub

Customer Impact

If the body of a Parallel.ForEachAsync loop throws an OperationCanceledException using the supplied CancellationToken and it hasn't had cancellation requested, the Task returned from ForEachAsync will never complete and the calling code will effectively hang, e.g.

await Parallel.ForEachAsync(source, async (item, cancellationToken) =>
{
    ...
    throw new OperationCanceledException(cancellationToken); // when !cancellationToken.IsCancellationRequested
});

This is not a common thing to do. However, if you do happen to do it, your app will hang. The code is filtering out such exceptions, but if they're the only ones, it ends up filtering out all of them, causing the call that would have completed the returned Task to throw an exception which is then inadvertently eaten.

The fix is to change the design to simply not do such filtering: all exceptions thrown in such a case will be stored in the task.

Testing

Additional unit tests.

Risk

Relatively low.

  • This is a new API in .NET 6.
  • The 99.9% use case is to just await the returned task, which this won't affect, as await only throws the first exception stored in the task.
  • The potential risk here is if someone is accessing the Task afterwards, they may see additional exceptions in Task.Exception.InnerExceptions that they didn't previously see.

Anipik and others added 3 commits September 15, 2021 16:22
If code in Parallel.ForEachAsync throws OperationCanceledExceptions containing the CancellationToken passed to the iteration and that token has _not_ had cancellation requested (so why are they throwing with it) and there are no other exceptions, the ForEachAsync will effectively hang after failing to complete the task returned from it.

The issue stems from how we treat cancellation.  If the user-supplied token hasn't been canceled but we have OperationCanceledExceptions for the token passed into the iteration (the "internal" token), it can only have been canceled because an exception occurred.  We filter out these cancellation exceptions, leaving just the exceptions that are deemed to have caused the failure in the first place.  But the code doesn't currently account for the possibility that the developer is (arguably erroneously) throwing such an OperationCanceledException with the internal cancellation token as that root failure. The fix is to only filter out these OCEs if there are other exceptions besides them.
@ghost
Copy link

ghost commented Sep 17, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@stephentoub stephentoub added the Servicing-consider Issue for next servicing release review label Sep 17, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone Sep 17, 2021
@danmoseley danmoseley changed the base branch from release/6.0 to release/6.0-rc2 September 17, 2021 01:51
@danmoseley
Copy link
Member

Didn't it rebase correctly?

@stephentoub
Copy link
Member

@jkotas jkotas deleted the backport/pr-59065-to-release/6.0 branch September 18, 2021 04:10
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Servicing-consider Issue for next servicing release review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants