Skip to content

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Sep 21, 2023

Fixes the tests in #50833 that started failing (3 of them hanging) when we ingested runtime updates, specifically this change dotnet/runtime#90253

We were blocking the send task, by blocking the request content, which is now being waited on in dispose. You could argue that dispose should timeout the send/receive tasks so that hanging HttpContent types can at least observe cancellation to try and unblock. See dotnet/runtime#92390 where that is brought up.

@BrennanConroy BrennanConroy added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Sep 21, 2023
@JamesNK
Copy link
Member

JamesNK commented Sep 21, 2023

The tests shouldn't need to do this. I think S.N.Q or HttpClient is wrong. See dotnet/runtime#92390

@BrennanConroy
Copy link
Member Author

The tests shouldn't need to do this

Shouldn't need to, but probably should anyways? Unless we're explicitly testing hanging in HttpContent, are we?

@JamesNK
Copy link
Member

JamesNK commented Sep 22, 2023

The client not completing the request is a legitimate scenario. We wouldn't have caught this bug if we were cleaning up everything correctly.

@BrennanConroy
Copy link
Member Author

I'm not disagreeing, but that should be an explicit test (preferably done by the client team).

@JamesNK
Copy link
Member

JamesNK commented Sep 22, 2023

POST_ServerCompletesWithoutReadingRequestBody_ClientGetsResponse could have CompleteStream added.

The other tests abort the client. Adding CompleteStream would make the test invalid.

@BrennanConroy BrennanConroy merged commit 85ae259 into main Sep 29, 2023
@BrennanConroy BrennanConroy deleted the brecon/http3 branch September 29, 2023 20:36
@ghost ghost added this to the 9.0-preview1 milestone Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants