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

x/net/http2/h2c: handler does not support http.ResponseController #71999

Closed
ainar-g opened this issue Feb 27, 2025 · 8 comments
Closed

x/net/http2/h2c: handler does not support http.ResponseController #71999

ainar-g opened this issue Feb 27, 2025 · 8 comments
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Feb 27, 2025

Go version

go version go1.23.6 linux/amd64

Output of go env in your module/workspace:

AR='ar'
CC='clang'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='clang++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOBIN=''
GOCACHE='REDACTED'
GOCACHEPROG=''
GODEBUG=''
GOENV='REDACTED'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build3067490271=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='REDACTED'
GOMODCACHE='REDACTED'
GONOPROXY='REDACTED'
GONOSUMDB='REDACTED'
GOOS='linux'
GOPATH='REDACTED'
GOPRIVATE='REDACTED'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='REDACTED'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='REDACTED'
GOTMPDIR=''
GOTOOLCHAIN='go1.23.6'
GOTOOLDIR='REDACTED'
GOVCS=''
GOVERSION='go1.23.6'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

Use a custom http.ResponseWriter that has the Unwrap() http.ResponseWriter method with h2c.NewHandler.

What did you see happen?

The handler didn't work properly, and with the GODEBUG=http2debug=2 env I see:

h2c: connection does not support Hijack

What did you expect to see?

The handler to work; the h2c handler to use http.NewResponseController to hijack a connection.

@gopherbot gopherbot added this to the Unreleased milestone Feb 27, 2025
@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 27, 2025
@prattmic prattmic added FeatureRequest Issues asking for a new feature that does not need a proposal. and removed BugReport Issues describing a possible bug in the Go implementation. labels Feb 28, 2025
@prattmic
Copy link
Member

cc @neild @tombergan

@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 28, 2025
@seankhliao
Copy link
Member

see #46319, hijack doesn't make sense for http2.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Feb 28, 2025
@ainar-g
Copy link
Contributor Author

ainar-g commented Feb 28, 2025

@seankhliao, I'm not sure what you mean by that.

https://cs.opensource.google/go/x/net/+/refs/tags/v0.35.0:http2/h2c/h2c.go;l=137

Unless I'm misreading something, this line clearly blocks h2cHandler from properly working with prior-knowledge HTTP/2, unless the response writer implements http.Hijacker.

@neild
Copy link
Contributor

neild commented Feb 28, 2025

Alternative: Let's just deprecate the h2c package. Go 1.24's net/http has direct support for unencrypted HTTP/2 connections (https://pkg.go.dev/net/http#Protocols.SetUnencryptedHTTP2) and there's no longer any good reason to keep this package around.

Filed proposal for that: https://go.dev/issue/72039

@neild
Copy link
Contributor

neild commented Feb 28, 2025

(Reopening this issue, though, because if we do want to keep the h2c package around, @ainar-g is right that it should use ResponseController to do the hijack.)

@neild neild reopened this Feb 28, 2025
@seankhliao
Copy link
Member

seankhliao commented Feb 28, 2025

ah I misread it as a hijack from a handler wrapped by h2c

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/655615 mentions this issue: http2/h2c: use ResponseController for hijacking connections

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest Issues asking for a new feature that does not need a proposal. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants