Improve abort handling #41263
Improve abort handling #41263
Conversation
ghost
added
the
| @@ -159,6 +159,11 @@ private async Task WriteBody(bool flush = false) | |||
| { | |||
| while (true) | |||
| { | |||
| if (_bodyOutput.Aborted) | |||
| { | |||
| break; | |||
There was a problem hiding this comment.
Not really sure this break here is ok, as I'm not really sure what the expectations are before/after this loop, is this what you had in mind @Tratcher ?
There was a problem hiding this comment.
We have tests explicitly checking that Writes after abort no-op, so I don't think we want to throw exceptions right?
There was a problem hiding this comment.
Right, Write shouldn't throw.
I think this check needs to be after ReadAsync or you'd miss an Abort that happened while waiting for data.
There was a problem hiding this comment.
Hmm, there might be an issue if AdvanceTo isn't called? Moving this check inside the Try block would cover that with the finally.
There was a problem hiding this comment.
Are we always calling Complete on the pipe? If we don't there's a memory leak.
| @@ -159,6 +159,11 @@ private async Task WriteBody(bool flush = false) | |||
| { | |||
| while (true) | |||
| { | |||
| if (_bodyOutput.Aborted) | |||
| { | |||
| break; | |||
There was a problem hiding this comment.
Hmm, there might be an issue if AdvanceTo isn't called? Moving this check inside the Try block would cover that with the finally.
You can skip AdvanceTo if you call Complete. Complete must always be called. |
|
The complete call is in the finally outside of the while loop, so I think we are good there. So should we just break at the top? I think the only question is where this new check for aborted needs to go |
|
I think the current version is fine. The important part is that the abort check is after ReadAsync. |
amcasey
added
area-networking
Fixes #31404