Add OperationCanceledException exceptions automatically #9362
Conversation
ghost
added
the
needs-area-label
|
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. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@stephentoub If the methods actually throw a TaskCanceledException, I guess it's still okay to catch OperationCanceledException when you await? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Yes, in fact we want folks to catch OperationCanceledException and not TaskCanceledException. From my perspective, our introducing TaskCanceledException was a well-intentioned mistake. The idea was that it would be an OperationCanceledException that would carry a little more information (the Task that was canceled) if necessary, but that additional information is basically never useful or needed, and it's often not even possible to provide, yet because APIs were throwing it, developers started writing catch blocks for it, which then made it a breaking change for the implementation throwing it to be tweaked in such a way that the base OperationCanceledException was being thrown. We should probably introduce an analyzer to warn people off from catching TaskCanceledException, and maybe even obsolete it (though that's drastic). |
gewarren
requested review from
a team,
AaronRobinsonMSFT,
David-Engel,
HongGit,
JRahnama,
Rick-Anderson,
ajcvickers,
halter73,
jkoritzinsky,
kouvel and
mconnew
as code owners
This comment was marked as outdated.
This comment was marked as outdated.
|
Based on @stephentoub 's comment, I checked our conceptual and samples repos. In conceptual docs, I found 138 instances of |
@BillWagner I'd say yes! |
|
Yes, please and thank you. |
xml/Microsoft.Extensions.Primitives/CancellationChangeToken.xml
Outdated
Show resolved
Hide resolved
xml/System.Runtime.CompilerServices/ConfiguredCancelableAsyncEnumerable`1.xml
Outdated
Show resolved
Hide resolved
|
Learn Build status updates of commit 55eac5d: ✅ Validation status: passed
This comment lists only the first 25 files in the pull request. For any questions, please:
|
3 participants
Replaces #8776, which had too many conflicts.