Skip to content

[release/5.0-rc2] Fix exception mapping during expect 100 cancellation. - #42014

Merged
ManickaP merged 3 commits into
release/5.0-rc2from
backport/pr-41800-to-release/5.0-rc2
Sep 15, 2020
Merged

[release/5.0-rc2] Fix exception mapping during expect 100 cancellation.#42014
ManickaP merged 3 commits into
release/5.0-rc2from
backport/pr-41800-to-release/5.0-rc2

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Sep 9, 2020

Copy link
Copy Markdown
Contributor

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.

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

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.

@ManickaP
ManickaP requested a review from a teamSeptember 9, 2020 12:46

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@danmoseley

Copy link
Copy Markdown
Contributor

Is this likely to have a customer impact? I'm guessing so - throwing something other than TaskCanceledException is highly unexpected?

@danmoseley

Copy link
Copy Markdown
Contributor

Test failure is #42024

@danmoseleydanmoseley added the Servicing-consider Issue for next servicing release review label Sep 9, 2020
@ManickaP

Copy link
Copy Markdown
Member

Is this likely to have a customer impact? I'm guessing so - throwing something other than TaskCanceledException is highly unexpected?

Yes, that is my concern and a reason I started this port.

@karelzkarelz added this to the 5.0.0 milestone Sep 10, 2020
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@ManickaP

Copy link
Copy Markdown
Member

All test failures unrelated, see #42024.
@scalablecory could you re-review pls.

@danmoseley

Copy link
Copy Markdown
Contributor

Is any test imaginable? Outside of stress.

@danmoseleydanmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 14, 2020
@danmoseley

Copy link
Copy Markdown
Contributor

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.

@ManickaP

Copy link
Copy Markdown
Member

@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.

@ManickaP
ManickaP merged commit f0b04a4 into release/5.0-rc2Sep 15, 2020
@ManickaP
ManickaP deleted the backport/pr-41800-to-release/5.0-rc2 branch September 15, 2020 18:36
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Net.HttpServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Dotnet-GitSync-Bot@danmoseley@ManickaP@karelz@wfurt