[HTTP/3] Reenabled ReadAsStreamAsync_HandlerProducesWellBehavedResponseStream #58041
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #53087 I ran 10_000 tight loops locally without an issue.
|
|
|
||
| if (connection is Http3LoopbackConnection http3Connection) | ||
| { | ||
| await http3Connection.WaitForClientDisconnectAsync(); |
There was a problem hiding this comment.
Just wondering if it's the same kind of a workaround we would have to add to other tests for #57779?
There was a problem hiding this comment.
I wonder if we should just add this to the HTTP3 version of AcceptConnectionAsync.
There was a problem hiding this comment.
Yes, it's the same workaround.
I tried to add it into AcceptConnectionAsync to make it more generic/future proof, but it caused timeouts in cancellation tests, so I scoped it to this particular test.
Side note: I think that the reason was that they dispose client one level higher then the server. Thus, the client connection is never closed before server needs to finish. Leading me to a conclusion that adding some generic solution might not be possible or at least won't be easy 😢
There was a problem hiding this comment.
Ha @geoffkizer I didn't notice your comment before posting mine. I tried that, it cause timeouts elsewhere.
There was a problem hiding this comment.
If it's only a handful of cancellation tests that failed, it might make more sense to update those tests to work with this than to update all the other tests to call WaitForClientDisconnectAsync.
Cancellation tests are tricky anyway; I'd rather be modifying them to be more robust than modifying a bunch of more straightforward tests...
Do you remember which cancellation tests failed?
There was a problem hiding this comment.
I'll have to retry. I think that the number of test instances was high but that it was just 2 or 3 test methods repeated for different inputs.
So do not merge on my behalf, I'll retry tomorrow and either try to fix those or report back how much of them it was.
There was a problem hiding this comment.
Another option would be to actually initiate server shutdown here by sending a GOAWAY, then call WaitForClientDisconnectAsync. This is probably better behavior, but more involved.
There was a problem hiding this comment.
I posted a PR to do the graceful shutdown in AcceptConnectionAsync -- #58088. Feedback welcome.
We should discuss if we want to take that PR or not.
ManickaP
force-pushed
the
mapichov/53087_reenable_test
branch
from
8f464b7
to
55a6f47
Compare
ghost
locked as resolved and limited conversation to collaborators
Fixes #53087
I ran 10_000 tight loops locally without an issue.