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

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

Comments

Projects
None yet
3 participants
@bradfitz
Member

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 this to the Go1.12 milestone Jul 9, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 9, 2018

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

@gopherbot

This comment has been minimized.

gopherbot commented Jul 9, 2018

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

net/http: remove dead code noted in post-submit review of CL 81778
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

This comment has been minimized.

Contributor

fraenkel commented Jul 10, 2018

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.

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