Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Apr 3, 2018

I couldn't get it to fail locally, but I believe the issue was that sporadically WinHttpHandler would not reuse the original connection, in which case it would either not close the first and the test would hang (and then the client would timeout) because nothing was accepting the second connection, or it would close the first and fail because the server was expecting a second request on the connection. This fixes it by using Connection: close on the first response so that the client won't reuse the same connection.

Fixes https://github.com/dotnet/corefx/issues/28537
Fixes https://github.com/dotnet/corefx/issues/25983
cc: @wfurt, @rmkerr

I couldn't get it to fail locally, but I believe the issue was that sporadically WinHttpHandler would not reuse the original connection, in which case it would either not close the first and the test would hang because nothing was accepting the second connection, or it would close the first and fail because the server was expecting a second request on the connection.  This fixes it by using Connection: close on the first response so that the client won't reuse the same connection.
Copy link
Contributor

@rmkerr rmkerr left a comment

Choose a reason for hiding this comment

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

LGTM. I wouldn't be surprised if we were running into this same issue in other tests too.

@davidsh
Copy link
Contributor

davidsh commented Apr 3, 2018

This fixes it by using Connection: close on the first response so that the client won't reuse the same connection.

Well-behaved servers will always send an HTTP response header that matches their connection behavior. So, if a server does close the connection after the request (despite the client wanting to do 'keep-alive'), the server will send 'Connection: close'. This helps the client figure out immediately that the connection is gone.

Much of our test code is written using the loopback server and doesn't follow these principles. It will close the connection after one request but doesn't send any 'Connection: close' response headers. In some ways, this is a good test to see if how resilient our client HTTP stack is. But as we've seen, there are race conditions and we end up having to do fixes like this.

Perhaps we should consider making our test code follow well-behaved patterns for servers and make sure the loopback server use is well-behaved.

@davidsh
Copy link
Contributor

davidsh commented Apr 3, 2018

cc: @geoffkizer

@stephentoub
Copy link
Member Author

Much of our test code is written using the loopback server and doesn't follow these principles. It will close the connection after one request but doesn't send any 'Connection: close' response headers. In some ways, this is a good test to see if how resilient our client HTTP stack is. But as we've seen, there are race conditions and we end up having to do fixes like this.

This case is a bit different. The server wasn't closing the connection, and was expecting the client to reuse it; the hang or failure would occur when the client didn't reuse it.

Regardless, yes, we need to be more cognizant of this in tests.

@stephentoub stephentoub merged commit 586cffc into dotnet:master Apr 3, 2018
@stephentoub stephentoub deleted the proxytest branch April 3, 2018 16:01
@davidsh
Copy link
Contributor

davidsh commented Apr 3, 2018

And according to the RFC, it is a MUST that the server send 'Connection: close' if it is going to close the connection.
https://tools.ietf.org/html/rfc7230#page-50

A server that does not support persistent connections MUST send the
"close" connection option in every response message that does not
have a 1xx (Informational) status code.

@karelz karelz modified the milestones: 2.2.0, 2.1.0 Apr 4, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…orefx#28770)

I couldn't get it to fail locally, but I believe the issue was that sporadically WinHttpHandler would not reuse the original connection, in which case it would either not close the first and the test would hang because nothing was accepting the second connection, or it would close the first and fail because the server was expecting a second request on the connection.  This fixes it by using Connection: close on the first response so that the client won't reuse the same connection.

Commit migrated from dotnet/corefx@586cffc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

4 participants