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

Write the header if none had been written in WriteResponse #6380

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Jun 7, 2024

In one of our modules we had this code:

	buf := new(bytes.Buffer)
	shouldBuffer := func(status int, header http.Header) bool {
		// Returning false means headers/body will be streamed out directly to the underlying writer.
		return false
	}
	ww := caddyhttp.NewResponseRecorder(w, buf, shouldBuffer)

	// Do stuff with ww
	// ...

	// Write response and return error if needed
	return ww.WriteResponse()

This worked, but when reading through the sources of caddy and the documentation of NewResponseRecorder we realized that we could also initialize it without the buffer and function:

ww := caddyhttp.NewResponseRecorder(w, nil, nil)

This matches how caddy initializes it, and is explicitly allowed in the docs.

However, with that change we noticed panics happening:

runtime.gopanic (/usr/local/go/src/runtime/panic.go:770)
runtime.panicmem (/usr/local/go/src/runtime/panic.go:261)
runtime.sigpanic (/usr/local/go/src/runtime/signal_unix.go:881)
bytes.(*Buffer).WriteTo (/usr/local/go/src/bytes/buffer.go:259)
io.copyBuffer (/usr/local/go/src/io/io.go:411)
io.Copy (/usr/local/go/src/io/io.go:388)
caddyhttp.(*responseRecorder).WriteResponse (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/responsewriter.go:229)
caddy.(*Telemetry).captureResponse (/src/caddy/telemetry.go:312)
caddy.(*Telemetry).ServeHTTP (/src/caddy/telemetry.go:152)
caddyhttp.wrapMiddleware.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddy.(*NewRelic).ServeHTTP.func1 (/src/caddy/newrelic.go:69)
http.HandlerFunc.ServeHTTP (/usr/local/go/src/net/http/server.go:2166)
newrelic.WrapHandle.func1 (/src/vendor/github.com/newrelic/go-agent/v3/newrelic/instrumentation.go:81)
http.HandlerFunc.ServeHTTP (/usr/local/go/src/net/http/server.go:2166)
newrelic.WrapHandleFunc.func1 (/src/vendor/github.com/newrelic/go-agent/v3/newrelic/instrumentation.go:150)
http.HandlerFunc.ServeHTTP (/usr/local/go/src/net/http/server.go:2166)
caddy.(*NewRelic).ServeHTTP (/src/caddy/newrelic.go:72)
caddyhttp.wrapMiddleware.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:331)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:300)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:268)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:268)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.RouteList.Compile.wrapRoute.func1.1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/routes.go:268)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.(*Server).enforcementHandler (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/server.go:435)
caddyhttp.(*App).Provision.(*Server).wrapPrimaryRoute.func1 (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/server.go:411)
caddyhttp.HandlerFunc.ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/caddyhttp.go:58)
caddyhttp.(*Server).ServeHTTP (/src/vendor/github.com/caddyserver/caddy/v2/modules/caddyhttp/server.go:347)
http.serverHandler.ServeHTTP (/usr/local/go/src/net/http/server.go:3137)
http.initALPNRequest.ServeHTTP (/usr/local/go/src/net/http/server.go:3745)
http2.(*serverConn).runHandler (/src/vendor/golang.org/x/net/http2/server.go:2370)
runtime.goexit (/usr/local/go/src/runtime/asm_amd64.s:1695)

Checking the code shows that this seems to a confusion inside the response recorder: rr.stream is set by WriteHeader, so checking that and then realizing that the status code is unset (i.e. WriteHeader hadn't been called!) is the wrong way around.

@ankon ankon changed the title Write the header if now had been written in WriteResponse Write the header if none had been written in WriteResponse Jun 7, 2024
@ankon ankon force-pushed the fix/responsewriter-null-buf-writeresponse-panic branch from 0e1e75c to dac9946 Compare June 7, 2024 13:00
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... I guess this makes sense. Thank you!

@mholt mholt merged commit 9be4f19 into caddyserver:master Jun 7, 2024
23 checks passed
@mholt mholt added the bug 🐞 Something isn't working label Jun 7, 2024
@mholt mholt added this to the v2.8.5 milestone Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants