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.

Sign up for GitHub

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

Jump to bottom

net/http: make Server.SetKeepAlivesEnabled(false) drop currently-open connections #9478

Closed
bradfitz opened this issue Dec 30, 2014 · 9 comments
Closed

net/http: make Server.SetKeepAlivesEnabled(false) drop currently-open connections #9478

bradfitz opened this issue Dec 30, 2014 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8Maybe

Comments

@bradfitz
Copy link
Contributor

When adjusting the firewall configuration on a GCE instance today, I was confused that I could toggle my new firewall rule and see the changes immediately affect my ability to telnet to the http server, but my browser continued to work fine, regardless of the firewall setting.

What I realized (too slowly) was happening was that once the keep-alive connection was open, the firewall permitted it forever, as designed and documented.

It would be nice of the google.golang.org/cloud/compute/metadata package or similar could listen for metadata changes (the GCE metadata service supports long polling on changes) to see when the firewall or tags change, and then call Server.SetKeepAlivesEnabled(false) to drop existing connections and force them to (try) to reconnect.

Currently Server.SetKeepAlivesEnabled only affects future connections and not active idle ones.
@bradfitz bradfitz added this to the Go1.5Maybe milestone Dec 30, 2014
@rsc
Copy link
Contributor

rsc commented Jun 29, 2015

Too late for Go 1.5.

@rsc rsc modified the milestones: Unplanned, Go1.5Maybe Jun 29, 2015
@jbardin jbardin mentioned this issue Mar 23, 2016
Closed
@bradfitz
Copy link
Contributor Author

Related: #4674

@tpng tpng mentioned this issue May 9, 2016
Closed
@tpng
Copy link

tpng commented May 9, 2016

Any plan on fixing this?
Default support for http2 is included in Go 1.6, and browsers supporting http2 are reusing single connection by design.
Server.SetKeepAlivesEnabled(false) only affecting new connection means there are currently no way to close the only connection opened by browsers gracefully.
We can close the connection using the Server.ConnState hook, but it causes Chrome to display a error page before opening a new connection.
And I don't think we have enough info to send a GOAWAY frame inside the Server.ConnState hook.

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned May 9, 2016
@tpng tpng mentioned this issue Jun 7, 2016
Merged
@FiloSottile FiloSottile mentioned this issue Aug 3, 2016
Closed
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016
@bradfitz bradfitz mentioned this issue Oct 17, 2016
Closed
@rsc
Copy link
Contributor

rsc commented Oct 20, 2016

@bradfitz, too late for Go 1.8, yes?

@bradfitz
Copy link
Contributor Author

A couple of the other issues might make this easy. I'll close this at the end of the week if it doesn't happen, but let's keep it open until then.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/32329 mentions this issue.

@bradfitz bradfitz mentioned this issue Nov 1, 2016
Closed
@bradfitz bradfitz reopened this Nov 3, 2016
@bradfitz
Copy link
Contributor Author

bradfitz commented Nov 3, 2016

I'm going to reopen this. The solution I have now is okay, but it's not great. It closes all currently-idle conns, but it should mark all conns for death as soon as they become idle. (For both http1 and http2).

I was thinking this before, but the flaky #17754 really drove it home.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/32587 mentions this issue.

gopherbot pushed a commit that referenced this issue Nov 3, 2016
@bradfitz
net/http: deflake TestServerSetKeepAlivesEnabledClosesConns
be7c50a 
Fixes #17754
Updates #9478 (details in here)

Change-Id: Iae2c1ca05a18ed266b53b2594c22fc57fab33c5e
Reviewed-on: https://go-review.googlesource.com/32587
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/33141 mentions this issue.

@golang golang locked and limited conversation to collaborators Nov 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8Maybe
Development

No branches or pull requests
5 participants
@bradfitz @rsc @quentinmit @tpng @gopherbot