[release/6.0] [HTTP/3] Abort response stream on dispose if content not finished #57741
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ntents are finsihed
…ding state within Send* methods.
ghost
added
the
|
Tagging subscribers to this area: @dotnet/ncl |
ManickaP
requested review from
danmoseley,
karelz,
CarnaViire,
wfurt and
geoffkizer
|
@adityamandaleeka @Tratcher @JamesNK this is one of your blockers -- is it sufficient in RC2? Or do you need it in RC1 as well? |
|
@karelz From the ASP.NET perspective, RC2 will work fine, but that said, we think the behavior here is bad enough to consider getting it into RC1 if possible (the client cancels a request and the server isn't notified, so pending reads will be hung until the connection is killed). |
|
When I was testing with the gRPC functional tests I believe I saw this issue cause the server to kill connections: The client canceled requests, and the unit test went off to do other things, while on the server the request hung around. Sometimes Kestrel DDOS protection would kick in a tear down the connection because data the server was writing wasn't being read by the client. I saw this a couple of days ago so I don't have exact details. It's not critical to fix immediately, but something to consider. |
|
@adityamandaleeka @JamesNK your responses on urgency for RC1 seem to be a bit conflicting to me ... |
|
The behavior and outcome are bad if someone cancels an HTTP request that is in the middle of downloading. I don't know how common it is for apps to do that. |
That reduces the risk for other scenarios, but we don't want to break HTTP/3 either so it would be good going forward to estimate the risk to that. Approved for release/6.0 due to high impact. As far as RC1 goes -- if it would give us more confidence that we are shipping the right change, it's probably worth taking to tactics. (GA quality being much more important than RC1 quality.) |
|
@danmoseley Risk is updated above. Can you please review? Thanks! |
|
Not needed, closing. The change is now in RC1 (via PR #57999) which will flow automatically to RC2 (release/6.0) branch. |
Backport of #57156 to release/6.0
Sends abort read/write if H/3 stream is disposed before respective contents are finished.
Fixes #56129
/cc @ManickaP
Customer Impact
Causes data loss / data corruption on server side.
Blocking ASP.NET (cc @adityamandaleeka)
Testing
CI (tests are part of the PR)
Local stress validation: 10,000 tight loop runs (without the fix it failed in ~500 iterations)
Risk
Low, H/3 only code path (hidden behind AppContext switch).
Impact on H/3 is Low/Medium -- the change is scoped only to error code path.