Skip to content
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: Server closeIdleConns does not close StateNew connections #22682

Closed
kcburge opened this issue Nov 13, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@kcburge
Copy link

commented Nov 13, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.9.2

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

any (linux x86_64)

What did you do?

Long Story: my web app was not closing when calling Shutdown on my http server. Google Chrome for some reason was creating a couple new connections to my server, but not making any requests on them. I'm guessing this is for performance, but I couldn't find any explanation. These extra connections stay in "StateNew" state, so, Shutdown does not close them when closing idle connections.

Short Story: Tried to "Shutdown" http server with StateNew connections.

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

What did you expect to see?

Since StateNew is an idle state, I'd expect it to be closed during Shutdown.

What did you see instead?

Shutdown must be cancelled to clear StateNew connections.

@kcburge kcburge changed the title http closeIdleConns does not close StateNew connections http Server closeIdleConns does not close StateNew connections Nov 13, 2017

@gbbr gbbr changed the title http Server closeIdleConns does not close StateNew connections net/http: Server closeIdleConns does not close StateNew connections Nov 13, 2017

@gbbr

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

The returned error is correct, it is of type contextDeadlineExceededError and is thrown because the context timed out. See here: https://play.golang.org/p/JmX0nEqeqa

@gbbr

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

From net/http/#Server.Shutdown:

Shutdown works by first closing all open listeners, then closing all idle connections, and then waiting indefinitely for connections to return to idle and then shut down. If the provided context expires before the shutdown is complete, Shutdown returns the context's error, otherwise it returns any error returned from closing the Server's underlying Listener(s).

@gbbr

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

As a potential solution to your problem, you might have more luck experimenting with the IdleConnTimeout setting from http.Transport. I and others would be more than happy to help if you post this as a question on the go-nuts mailling list.

Closing, since there is no bug.

@gbbr gbbr closed this Nov 13, 2017

@kcburge

This comment has been minimized.

Copy link
Author

commented Nov 13, 2017

Thanks for the quick response. I apologize if the problem description and example did not make the issue clear. The problem is that a new, unused connection is NOT considered "idle" by closeIdleConns and is therefore NOT closed during Shutdown. The only reason I added a context with a timer to my example was to clearly show that the idle connection was not being closed, instead, the context is timing out. The only difference between StateNew and StateIdle is that a StateNew connection has never made a request, correct? So, as you quoted "Shutdown works by ... closing all idle connections..." A StateNew is an idle connection, is it not?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

Since StateNew is an idle state

Go has StateNew different from StateIdle intentionally. New isn't idle. A new connection is expected to have a request sent on it soon, and browsers have certain expectations of the server not hanging up on them for new connections. But keep-alive (idle) connections are much more likely to be closed at random times, and clients expect this more.

It's intentional that Shutdown doesn't close StateNew connections.

But if Chrome is keep some warmed up TCP connections in StateNew, maybe we need to do something. Maybe close StateNew after some duration (~2-5 seconds?).

/cc @tombergan

@bradfitz bradfitz added this to the Go1.11 milestone Nov 13, 2017

@gbbr gbbr removed the ExpertNeeded label Nov 13, 2017

@kcburge

This comment has been minimized.

Copy link
Author

commented Nov 14, 2017

This is somewhat duplicated by #21204

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

There are cases where clients will leave a connection open and not use it. For example: an HTTP/1 client might start a new connection but switch to an existing connection if one becomes idle before the dial completes. If that happens, and there are no more requests to send, the new connection will sit around unused. Clients might also "preconnect" to origins that the client predicts it will visit shortly.

@kcburge, have you tried using ReadHeaderTimeout? I think that will solve your problem. If you set ReadHeaderTimeout to 10s, then Shutdown should return within 10s after all outstanding requests complete.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

From #21204, @pamburus points out that ReadHeaderTimeout is infinite by default. As a result, the default behavior of Shutdown is to block indefinitely when there are StateNew connections. This seems undesirable.

@bradfitz
It's intentional that Shutdown doesn't close StateNew connections.

Do you expect anything will break if Shutdown closes StateNew connections? The only possible breakage I can imagine is if we close a connection while a POST is in flight -- the client won't know if we processed any of the POST and thus cannot know if it's safe to retry the POST. This doesn't seem very significant. It might be safe to close StateNew connections immediately in Shutdown.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

I'm fine closing them if they're a few seconds old.

My concern was closing a request that was just being sent.

@kcburge

This comment has been minimized.

Copy link
Author

commented Nov 23, 2017

I'm not an expert in writing http servers, but, closing the State New connections either "now" (immediately when Shutdown is called, in closeIdleConns), or at some time later (through ReadHeaderTimeout or during Context cancellation, one of which is required by the current implementation), only differs as to WHEN the connection is closed: either way, the connection could have a POST in flight, or the client could be about to submit a request, or.... So, this makes the discussion about those things moot: either way, the connection is going to be closed. That's why I asked why make the server wait if it knows nothing has happened on the socket. Thanks for looking at this.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 28, 2018

Change https://golang.org/cl/121419 mentions this issue: net/http: make Server.Shutdown treat new connections as idle after 5 seconds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.