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/httputil: ReverseProxy implicitly requires http.Flusher with streaming #64045

Closed
vhakulinen opened this issue Nov 9, 2023 · 5 comments
Closed

net/http/httputil: ReverseProxy implicitly requires http.Flusher with streaming #64045

vhakulinen opened this issue Nov 9, 2023 · 5 comments
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@vhakulinen
Copy link

vhakulinen commented Nov 9, 2023 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.21.4 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/ville/.cache/go-build'
GOENV='/home/ville/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/ville/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/ville/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/ville/.local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/ville/.local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.4'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/t
mp/go-build3935927459=/tmp/go-build -gno-record-gcc-switches'

What did you do?

httputil.ReverseProxy requires that the used http.ResponseWriter implements http.Flush in order to proxy SSE.

I have a situation where the original response writer is wrapped with the following code in order to record the written status code for logging:

// StatusRecorder wraps http.ResponseWriter and records the status code
// written to the client.
type StatusRecorder struct {
	http.ResponseWriter
	Status int
}

func NewStatusRecorder(w http.ResponseWriter) *StatusRecorder {
	return &StatusRecorder{
		ResponseWriter: w,
		// Default to 200 OK.
		Status: http.StatusOK,
	}
}

func (rr *StatusRecorder) WriteHeader(status int) {
	rr.Status = status
	rr.ResponseWriter.WriteHeader(status)
}

What did you expect to see?

SSE events landing to the client or mention about the http.Flush requirement in the reverse proxy's documentation.

What did you see instead?

SSE events were lost.

Similar issue: #27816
@seankhliao
Copy link
Member

I think it's just expected that any implementation of wrapping responsewriters will have an Unwrap() http.ResponseWriter method.

@seankhliao seankhliao added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 9, 2023
@vhakulinen
Copy link
Author

vhakulinen commented Nov 9, 2023 •
edited

I have completely missed that. I've only been aware of Unwrap() when working with errors.

Is it common practice to implement Unwrap() whenever wrapping interfaces, or just these two cases?

@seankhliao
Copy link
Member

afaik these are the only two the standard library supports atm.

@seankhliao
Copy link
Member

closing as I don't think this is specific to httputil.ReverseProxy, it would have broken with any other non conforming middleware as well.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Dec 29, 2023
@vhakulinen
Copy link
Author

Okay. Thank you for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@vhakulinen @seankhliao