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

Should middleware.compressResponseWriter implements the Unwrap method ? #818

Closed
kei2100 opened this issue Apr 27, 2023 · 1 comment
Closed

Comments

@kei2100
Copy link
Contributor

kei2100 commented Apr 27, 2023

When using http.ResponseController introduced in Go 1.20 with middleware.Compress, an error occurs.

Example of reproduce: https://go.dev/play/p/z_auq0KiEBx
package main

import (
	"fmt"
	"net/http"
	"net/http/httptest"
	"testing"
	"time"

	"github.com/go-chi/chi/v5/middleware"
)

func TestResponseController(t *testing.T) {
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		// use http.ResponseController to customize read deadline
		rc := http.NewResponseController(w)
		if err := rc.SetReadDeadline(time.Now().Add(time.Minute)); err != nil {
			fmt.Printf("failed to set read deadline: %+v", err)
			w.WriteHeader(500)
			return
		}
		w.WriteHeader(200)
	})

	t.Run("works", func(t *testing.T) {
		svr := httptest.NewServer(
			handler,
		)
		t.Cleanup(svr.Close)
		resp, _ := http.Get(svr.URL)
		// assert status code
		if g, w := resp.StatusCode, 200; g != w {
			t.Errorf("\ngot :%v\nwant:%v", g, w)
		}
	})

	t.Run("works with the compress middleware", func(t *testing.T) {
		// use compress middleware
		svr := httptest.NewServer(
			middleware.NewCompressor(6).Handler(handler),
		)
		t.Cleanup(svr.Close)
		resp, _ := http.Get(svr.URL)
		// assert status code
		if g, w := resp.StatusCode, 200; g != w {
			t.Errorf("\ngot :%v\nwant:%v", g, w)
		}
	})
}

Results:

=== RUN   TestResponseController
=== RUN   TestResponseController/works
=== RUN   TestResponseController/works_with_the_compress_middleware
failed to set read deadline: feature not supported    prog_test.go:46: 
        got :500
        want:200
--- FAIL: TestResponseController (30.00s)
    --- PASS: TestResponseController/works (30.00s)
    --- FAIL: TestResponseController/works_with_the_compress_middleware (0.00s)
FAIL

This is because http.ResponseController requires ResponseWriter to implement the Unwrap method that returns the original ResponseWriter. See the following code in Go 1.20 source code:

https://cs.opensource.google/go/go/+/refs/tags/go1.20.3:src/net/http/responsecontroller.go;l=23-24

// The ResponseWriter should be the original value passed to the Handler.ServeHTTP method,
// or have an Unwrap method returning the original ResponseWriter.

https://cs.opensource.google/go/go/+/refs/tags/go1.20.3:src/net/http/responsecontroller.go;l=79-96

// SetReadDeadline sets the deadline for reading the entire request, including the body.
// Reads from the request body after the deadline has been exceeded will return an error.
// A zero value means no deadline.
//
// Setting the read deadline after it has been exceeded will not extend it.
func (c *ResponseController) SetReadDeadline(deadline time.Time) error {
	rw := c.rw
	for {
		switch t := rw.(type) {
		case interface{ SetReadDeadline(time.Time) error }:
			return t.SetReadDeadline(deadline)
		case rwUnwrapper:
			rw = t.Unwrap()
		default:
			return errNotSupported()
		}
	}
}

To solve this issue, we need to implement the Unwrap method in middleware.compressResponseWriter.

Note that middleware.basicWriter, which is returned by middleware.NewWrapResponseWriter, already implements the Unwrap method, so this issue does not occur with it.

@kei2100
Copy link
Contributor Author

kei2100 commented Jul 14, 2023

Fixed in https://github.com/go-chi/chi/releases/tag/v5.0.10

middleware.Compress: implement Unwrap() method to the compressResponseWriter -- #819

@kei2100 kei2100 closed this as completed Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant