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: verify that net/http.Server.SetKeepAlivesEnabled(false) shuts down HTTP/2 server #26303

Open
bradfitz opened this issue Jul 9, 2018 · 3 comments
Open

x/net/http2: verify that net/http.Server.SetKeepAlivesEnabled(false) shuts down HTTP/2 server #26303

bradfitz opened this issue Jul 9, 2018 · 3 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Jul 9, 2018

Per #20239 (comment) and its immediate reply, it seems that https://go-review.googlesource.com/c/net/+/43230 might've broken net/http.Server.SetKeepAlivesEnabled(false) shutting down HTTP/2 server connections.

I guess there's no test for it.

/cc @tombergan @pam4
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 9, 2018
@bradfitz bradfitz added this to the Go1.12 milestone Jul 9, 2018
@bradfitz
Copy link
Contributor Author

bradfitz commented Jul 9, 2018

There is TestServerSetKeepAlivesEnabledClosesConns in serve_test.go, but it's only for HTTP/1.

@gopherbot
Copy link

Change https://golang.org/cl/122820 mentions this issue: net/http: remove dead code noted in post-submit review of CL 81778

gopherbot pushed a commit that referenced this issue Jul 10, 2018
@bradfitz
net/http: remove dead code noted in post-submit review of CL 81778
c793ea3 
Per comments in #20239 (comment)

Updates #20239
Updates #26303

Change-Id: Iddf34c0452bd30ca9111b951bca48d1e011bd85a
Reviewed-on: https://go-review.googlesource.com/122820
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@fraenkel
Copy link
Contributor

This is actually difficult to test given how the current code works.
The h1ServerKeepAlivesDisabled checks for an interface which only works if its executed within the http package.

@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@fraenkel fraenkel mentioned this issue Feb 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@bradfitz @rsc @andybons @fraenkel @gopherbot