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

x/net/http2/h2c: support closure of all connections and graceful shutdown #26682

Open
nhooyr opened this issue Jul 30, 2018 · 7 comments
Open

x/net/http2/h2c: support closure of all connections and graceful shutdown #26682

nhooyr opened this issue Jul 30, 2018 · 7 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Jul 30, 2018 •
edited

Very happy to see h2c supported: golang/net@c4299a1

However, there is no way to enable graceful shutdown, or even close all h2c connections. We should support this.

You could use http2.ConfigureServer to configure a http2 server's graceful shutdown to be called when the http server is shutdown but the http server's shutdown method will not wait for the http2 connections to close because they have been hijacked.

I think it'd be simplest if we added a Shutdown and Close method to *http2.Server analogous to *http.Server's methods.
@nhooyr nhooyr changed the title x/net/http2: support closure of all connections and graceful shutdown x/net/http2/h2c: support closure of all connections and graceful shutdown Jul 30, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jul 30, 2018
@nhooyr nhooyr changed the title x/net/http2/h2c: support closure of all connections and graceful shutdown http2/h2c: support closure of all connections and graceful shutdown Jul 30, 2018
@obeattie
Copy link

Is there a workaround possible for this? As far as I can see it's completely impossible to do at present, as the methods that need to be invoked to gracefully shutdown a connection (eg. serverConn.startGracefulShutdown) are unexported. cc. @bradfitz

@obeattie
Copy link

After poking a bit more, it looks like I can do something like this, which is a pretty horrid hack but does appear to work:

h2s := &http2.Server{}
h1s := &http.Server{}
http2.ConfigureServer(h1s, h2s)
// Make use of h2s in a h2c handler…
h1s.Shutdown(ctx)

This works because http2.ConfigureServer adds a shutdown hook into the http.Server such that when its Shutdown() method is invoked, the private startGracefulShutdown method is called. Shutdown() calls this method regardless of the fact that h1s was never "used."

@nhooyr
Copy link
Contributor Author

nhooyr commented Oct 22, 2018

That does not work as the Shutdown itself doesn't wait for the h2c connections.

See

go/src/net/http/server.go

Line 2631 in 86ce1cb

go f()

It starts the graceful shutdown of h2c connections in a new goroutine but only wait for the unhijacked connections.

@agnivade agnivade changed the title http2/h2c: support closure of all connections and graceful shutdown x/net/http2/h2c: support closure of all connections and graceful shutdown Oct 23, 2018
@agnivade agnivade added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Oct 23, 2018
@bradfitz
Copy link
Contributor

Sorry, h2c is super low on my priority list. I don't have time to investigate this enough to be helpful here.

obeattie added a commit to monzo/typhon that referenced this issue Oct 25, 2018
@obeattie
Connection draining for h2c connections 🚽
e308b36 
Connections that were upgraded to HTTP/2 by use of the H2cFilter can now be drained properly. The implementation is pretty ugly because Go does not have native support for connection draining on h2c connections, and as per golang/go#26682 this isn't a priority for the project.
obeattie added a commit to monzo/typhon that referenced this issue Oct 28, 2018
@obeattie
Connection draining for h2c connections 🚽
c3902a4 
Connections that were upgraded to HTTP/2 by use of the H2cFilter can now be drained properly. The implementation is pretty ugly because Go does not have native support for connection draining on h2c connections, and as per golang/go#26682 this isn't a priority for the project.
@obeattie obeattie mentioned this issue Oct 28, 2018
Merged
obeattie added a commit to monzo/typhon that referenced this issue Nov 15, 2018
@obeattie
Connection draining for h2c connections 🚽
842491d 
Connections that were upgraded to HTTP/2 by use of the H2cFilter can now be drained properly. The implementation is pretty ugly because Go does not have native support for connection draining on h2c connections, and as per golang/go#26682 this isn't a priority for the project.
obeattie added a commit to monzo/typhon that referenced this issue Nov 16, 2018
@obeattie
Connection draining for h2c connections 🚽
1569ba3 
Connections that were upgraded to HTTP/2 by use of the H2cFilter can now be drained properly. The implementation is pretty ugly because Go does not have native support for connection draining on h2c connections, and as per golang/go#26682 this isn't a priority for the project.
@gopherbot
Copy link

Change https://golang.org/cl/185059 mentions this issue: http2: add Close and Shutdown functions to the Server.

@rsc rsc mentioned this issue May 22, 2020
Open
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Oct 1, 2021
@AlexanderYastrebov
Adds h2c support
fd59982 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Oct 1, 2021
Closed
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Oct 14, 2021
@AlexanderYastrebov
Adds h2c support
b019978 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Oct 14, 2021
Open
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Dec 22, 2021
@AlexanderYastrebov
Adds h2c support
47eb635 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Dec 22, 2021
@AlexanderYastrebov
Adds h2c support
50cb57b 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Dec 22, 2021
@AlexanderYastrebov
Adds h2c support
e052456 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Dec 22, 2021
@AlexanderYastrebov
Adds h2c support
d3103d1 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Dec 22, 2021
@AlexanderYastrebov
Adds h2c support
0d9432d 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Dec 22, 2021
@AlexanderYastrebov
Adds h2c support
f2b780b 
Enables HTTP/2 connections over cleartext TCP with Prior Knowledge (RFC 7540 3.4).

The implementation is based on the golang.org/x/net/http2/h2c and workarounds several issues:
* golang/go#38064
* golang/go#26682

See h2c package docs for details.

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@akshayjshah
Copy link
Contributor

Would the Go team be open to CLs that address this? (I see that there's a very old CL open, but it doesn't seem to have been reviewed.) h2c is particularly popular in the gRPC ecosystem, and it'd be nice for gRPC implementations built on net/http to support graceful shutdown. We've had a few Connect users complain about this 😢

Is @neild the right person to take a look at x/net/http and net/http issues?

AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 24, 2023
@AlexanderYastrebov
skipper: support h2c
55045ae 
Work around golang/go#26682 by introducing
ShutdownListener that tracks active connections and implements graceful
shutdown.

For #1253
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 24, 2023
@AlexanderYastrebov
skipper: support http2 over cleartext TCP (h2c)
62b249d 
Works around golang/go#26682 by introducing
ShutdownListener that tracks active connections and implements graceful
shutdown.

For #1253
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Jul 24, 2023
Open
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 24, 2023
@AlexanderYastrebov
skipper: support http2 over cleartext TCP (h2c)
bbd6747 
Works around golang/go#26682 by introducing
ShutdownListener that tracks active connections and implements graceful
shutdown.

For #1253

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 24, 2023
@AlexanderYastrebov
skipper: support http2 over cleartext TCP (h2c)
ff6f242 
Works around golang/go#26682 by introducing
ShutdownListener that tracks active connections and implements graceful
shutdown.

For #1253

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov
Copy link
Contributor

That does not work as the Shutdown itself doesn't wait for the h2c connections.
It starts the graceful shutdown of h2c connections in a new goroutine but only wait for the unhijacked connections.

I think I've come up with a workaround. The idea is to use custom listener that would wait until connections are closed, see zalando/skipper#2480

AlexanderYastrebov added a commit to zalando/skipper that referenced this issue Jul 25, 2023
@AlexanderYastrebov
skipper: support http2 over cleartext TCP (h2c)
3a9dcac 
Works around golang/go#26682 by introducing
ShutdownListener that tracks active connections and implements graceful
shutdown.

For #1253

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@bradfitz @obeattie @AlexanderYastrebov @akshayjshah @agnivade @gopherbot @nhooyr