[release/5.0-rc2] Fix exception mapping during expect 100 cancellation. #42014
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
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. |
|
Is this likely to have a customer impact? I'm guessing so - throwing something other than TaskCanceledException is highly unexpected? |
|
Test failure is #42024 |
Yes, that is my concern and a reason I started this port. |
|
Tagging subscribers to this area: @dotnet/ncl |
|
All test failures unrelated, see #42024. |
|
Is any test imaginable? Outside of stress. |
danmoseley
added
Servicing-approved
|
Approved in email. It would be good to consider how to test this -- in System.Threading there are quite a few unit tests that successfully protect fixes for bugs that rarely repro using lots of looping and tricks. I do not know whether that is possible here. That need not hold up merging this though. |
|
@danmosemsft This is a timing issue, you need to fire the cancellation at a very precise moment and process everything around it in a certain order. Even if I were to change production code to manually control the timing, it would still be complicated. |
ghost
locked as resolved and limited conversation to collaborators
Backport of #41800 to release/5.0-rc2
Contributes to #40388
/cc @ManickaP
Customer Impact
The related code block has been introduced in 5.0 (in PR #38774) and there is a regression from 3.1: a different type of exception is thrown for cancellation than
TaskCancelledException. This could potentially break existing code that works on 3.1.Testing
Discovered during HTTP stress tests, eg.: Run HttpStress - HTTP 1.1. It's been occurring on every HTTP 1.1 run, after the fix, the error disappeared.
Risk
Low. Also this affects only newly introduced code in 5.0.