New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: half-closed connection triggers request cancellation #18527

Open
benburkert opened this Issue Jan 5, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@benburkert
Contributor

benburkert commented Jan 5, 2017

Please answer these questions before submitting your issue. Thanks!

What did you do?

https://play.golang.org/p/DnXwH5qJkD

What did you expect to see?

A client that sends a request followed immediately by a FIN can read a response from the server.

What did you see instead?

The half-close is detected and the request is cancelled immediately.

Does this issue reproduce with the latest release (go1.7.4)?

No

System details

go version go1.8beta2 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/benburkert"
GORACE=""
GOROOT="/Users/benburkert/sdk/go1.8beta2"
GOTOOLDIR="/Users/benburkert/sdk/go1.8beta2/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/qs/qkt9twmx4qg379d6f8kxl1vm0000gn/T/go-build530443777=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
GOROOT/bin/go version: go version go1.8beta2 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.8beta2 X:framepointer
uname -v: Darwin Kernel Version 16.3.0: Thu Nov 17 20:23:58 PST 2016; root:xnu-3789.31.2~1/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.12.2
BuildVersion:	16C67
lldb --version: lldb-360.1.70
@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

Is this theoretical or does any popular client in the wild actually use half-closed TCP or TLS connections?

I considered this but couldn't find a problem in practice.

@benburkert

This comment has been minimized.

Contributor

benburkert commented Jan 6, 2017

We have an internal client that fires-and-forget's requests to a service. Those requests may take some time to process, and the server uses the request's contexts for timeouts unrelated to the client connection. AFAIK there is no way to prevent the "client gone" detection from canceling the context.

Other web servers do support this behavior, but it's not covered by any RFC: http://mailman.nginx.org/pipermail/nginx/2008-September/007388.html

Supporting clients that want to fire-and-forget requests could be added by ignoring the read EOF if the request sets the Connection: close header. But this is also a behavior unspecified by any RFC. Another alternative is to add an "ignore client gone" field to Server.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

What do you mean by "fire-and-forget"? It goes out of its way to do a shutdown on its TCP connection, rather than writing the request body, reading the response, and calling close once? I've never seen any client do that.

Thanks for the nginx link.

@mnot, is there any HTTP RFC which says how clients and/or servers should behave here with respect to half-closed (closed by client) requests?

@bradfitz bradfitz added this to the Go1.8Maybe milestone Jan 6, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

I'm tagging this Go1.8Maybe to think about more, but I think it's too late to change anything here. The workaround is your application can ignore the Handler's Request.Context(). Or you can remove the shutdown from your internal HTTP client, IIUC.

@mnot

This comment has been minimized.

mnot commented Jan 6, 2017

The relevant part of the specs is:
http://httpwg.org/specs/rfc7230.html#connection.management

... but that doesn't address this situation. If you think it should, log an issue in https://github.com/httpwg/http11bis

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 6, 2017

@benburkert, is your internal client making HTTP/1.0 requests? (See httpwg/http-core#22 (comment))

I'm still trying to understand what your client's motivation is.

@rsc

This comment has been minimized.

Contributor

rsc commented Jan 9, 2017

@benburkert

This comment has been minimized.

Contributor

benburkert commented Jan 9, 2017

I described the issue as a problem with a client that depends on unspecified behavior hoping to clarify the issues but it seems to have had the opposite effect. Clients should not depend on unspecified behavior and we will fix our client to not half-close connections.

However, from a server's perspective normal browsers behavior can look identical to the fire-and-forget client I described above: a user closes their browser soon after initiating request for an asset. The server sees a connection, a valid request, and a FIN packet in very short succession.

Deciding to abort the request/response isn't so straight forward. A pageview endpoint would likely want to proceed without cancellation so that the page load is recorded. A large asset endpoint probably wants to drop the response as soon as the FIN is detected.

Because the HTTP spec does not cover handling a client that terminates the TCP connection unexpectedly, it seems that this it is left up to the library and application developers. I'm in favor of net/http deciding that an early EOF from the client triggers a cancellation of the request's context since it is ultimately up to the author of the handler to use the context or not.

But a handler that uses the request's context can behave differently between 1.7 and 1.8 due to EOF triggered cancellations. This probably won't be noticed by most but pageview style handlers will be affected, along with proxying handlers that want to let the backend decide how to handle an early client EOF.

I'm in favor of closing this issue because, even if there were a non-hacky way to support the 1.7 behavior, as @bradfitz said it's too late in the release cycle to do anything about it. The few handlers that are effected by this will have to be update to be more explicit about how they handle context cancellation, which seems fine.

@benburkert

This comment has been minimized.

Contributor

benburkert commented Jan 9, 2017

one last thought: it may be benifitial to use a custom context.WithCancel so that the error on a early client EOF can be something other than context.Canceled. I believe that would make it easier to filter out this event from other context cancellations, though I haven't given it much thought.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jan 9, 2017

I went to add support for not canceling contexts for HTTP/1.0 POST/PUT requests, wrote a test I expected to fail but then I discovered Go didn't read the request body which confused me for a second until I also discovered that HTTP/1.0 always required a Content-Length (httpwg/http-core#22 (comment)).

So I'm inclined to do nothing here, at least for now.

@benburkert, I agree that some Handlers may want to continue processing after the client has gone away. But like you said, they can use a different Context instead.

We can do the release candidate and see how things go. We can revisit this if there are problems.

@bradfitz bradfitz closed this Jan 9, 2017

@DanielMorsing

This comment has been minimized.

Contributor

DanielMorsing commented Dec 19, 2017

I encountered a variation on this bug today. It turns out a big-name CDN actually does do half-close on http/1.1.

I'm not sure how you'd detect a half-closed TCP connection. We could assume that the request end being closed when Connection: close is present means that the Close is a half one, but that leaves us with no way of detecting a regular close for the common case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment