Special characters added to pages due to compression on v5.0.13

[surakshith-suvarna]

In previous versions, compression was only applied if we had set the Content-Type header (w.Header().Set("Content-Type", "text/html")). In version 5.0.13 if the header is not set ������� displays on every page where the header is missing.

[balogal]

Hi, I'm experiencing the same when upgrading from v5.0.12 to v5.0.13. The extra bytes seem to be added if the content type of the response is not among the ones that should be compressed.

The following code snippet reproduces the issue:

func TestCompressionResponse(t *testing.T) {
	yamlDoc := []byte("key: value")

	handler := func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Content-Type", "application/x-yaml")
		w.Write(yamlDoc)
	}

	r := chi.NewRouter()
	r.Use(middleware.Compress(5))
	r.Get("/test", handler)
	srv := &http.Server{Addr: ":3333", Handler: r}
	go srv.ListenAndServe()

	res, err := http.Get("http://localhost:3333/test")
	if err != nil {
		t.Fatalf("could not send request: %v", err)
	}

	got, err := io.ReadAll(res.Body)
	if err != nil {
		t.Fatalf("could not read response body: %v", err)
	}

	if !bytes.Equal(got, yamlDoc) {
		// v5.0.12: pass
		// v5.0.13: server_test.go:39: expected "key: value" but got "key: value\x1f\x8b\b\x00\x00\x00\x00\x00\x00\xff\x01\x00\x00\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00"
		t.Errorf("expected %q but got %q", yamlDoc, got)
	}
}

[VojtechVitek]

Thanks for reporting this issue. I'll revert #919 via #924.

I wonder if anyone could look into the root cause of the issue introduced by #919. I doesn't make sense to me. @Neurostep?

[VojtechVitek]

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

[VojtechVitek]

@balogal would you be able to submit a similar compress mw test case to chi, please?

[balogal]

Thanks for the quick reaction @VojtechVitek. I'll have a shot at the test.

[js-everts]

Those special characters are the GZIP File header. Calling Close() on gzip.Writer will make it write the GZIP File Header even if no data had passed through it. The same is true for DEFLATE.

The changes introduced in #919 meant that Close() was called on the wrapped compress writer every single time regardless of weather data was compressed or not.

[Neurostep]

@surakshith-suvarna that is a great finding 👍 Didn’t know, gzip/deflate writes when closing even though there were no prior writes, TIL

@VojtechVitek So now we have a controversial situation, when for one case the writer has to call Close() to release resources, and another case when we should refrain from calling this as it breaks the normal flow. I wonder if we can find a way to not wrap the http.ResponseWriter at all in case we encounter that the content shouldn’t be compressed? Now we wrap it unconditionally and do the checking later. If this sounds like a way forward, I can look into this later next week, starting Wednesday.

@VojtechVitek @pkieltyka Please, let me know what do you think

[VojtechVitek]

Sounds good. This is delicate and we need more test cases.

I propose we wrap the ResponseWriter with compress writer during the first .Write() with non-empty body.

So,

1. We would not wrap with compress on:

w.WriteHeader(200)
w.Write(nil)

2. We'd check if we could wrap on the first non-empty .Write() based on Content-Type:

// w.WriteHeader(200)
w.Write(body) // wrap orig. ResponseWriter with compress writer if the Content-Type allows it

[Neurostep]

@VojtechVitek

Just came back from my PTO and wanna quickly follow up here.

I have read your proposal above and I find it very hard and un-intuitive to implement the way it is described. I believe the compressResponseWriter is deliberately coupled with the http.ResponseWriter and currently we wrap the original writer with compress writer at the very beginning of the middleware.

My idea was to move the isCompressable logic to the Compressor data type, instead of the compressResponseWriter. Then, we would check if the content is compressible (contains expected Content-Type) we would wrap the http.ResponseWriter with compressResponseWriter otherwise we would continue without wrapping. This way we wouldn't even create/retrieve the encoder and simplify logic around isCompressible and avoid logic about the first Write and Write(nil).

Am I missing something? 🤔

[VojtechVitek]

@Neurostep ok sounds good :)

[Neurostep]

@VojtechVitek @balogal @js-everts For posterity, I created the following PR #928 to mitigate the memory leak issue and at the same time keep in mind this issue.

It would be great if anyone could give feedback for the mentioned PR 🙏