Using handler.CompressHandler with mux.NotFoundHandler

[wayneashleyberry]

Hey,

I seem to be getting errors when using a CompressHandler in conjunction with a NotFoundHandler. In the following example my 404's don't work at all and the browser's just dumping a "This site can’t be reached" error to the screen. No error messages or stack traces coming from Go either :/

If I comment out r.NotFoundHandler or h2 := handlers.CompressHandler(h1) then it works.

package main

import (
    "fmt"
    "net/http"
    "os"

    "github.com/gorilla/handlers"
    "github.com/gorilla/mux"
)

func main() {
    r := mux.NewRouter()

    r.HandleFunc("/", IndexHandler).Methods("GET")
    r.HandleFunc("/test", TestHandler).Methods("GET")
    r.NotFoundHandler = http.HandlerFunc(NotFoundHandler)

    h1 := handlers.CombinedLoggingHandler(os.Stdout, r)
    h2 := handlers.CompressHandler(h1)

    http.ListenAndServe("localhost:"+os.Getenv("PORT"), h2)
}

func NotFoundHandler(w http.ResponseWriter, r *http.Request) {
    w.WriteHeader(http.StatusNotFound)
    fmt.Fprintf(w, "404 - Not Found")
}

func IndexHandler(w http.ResponseWriter, r *http.Request) {
    fmt.Fprintf(w, "Hello, World!")
}

func TestHandler(w http.ResponseWriter, r *http.Request) {
    fmt.Fprintf(w, "Looks good!")
}

[wayneashleyberry]

Looks like the call to w.WriteHeader() is causing the problem, but without it my 404 pages have a 200 status code.

[elithrar]

Interesting. Go 1.7?

[wayneashleyberry]

Yup, 1.7

[wayneashleyberry]

Yeah, this is very strange. I've just found if I leave the browser error open for a while I get asked if I want to allow the site to download multiple files... but nothing actually downloads.

Here's what curl gets:

~
🍡  curl -X GET http://localhost:3000/foo -v
*   Trying ::1...
* Connected to localhost (::1) port 3000 (#0)
> GET /foo HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.43.0
> Accept: */*
>
< HTTP/1.1 404 Not Found
< Content-Type: application/x-gzip
< Date: Mon, 05 Sep 2016 07:30:15 GMT
< Vary: Accept-Encoding
< Content-Length: 15
<
* Connection #0 to host localhost left intact
404 - Not Found

[wayneashleyberry]

Looks like compress.go#L25 is the problem, commenting it out fixes this issue. Out of interest, why would the compress handler need to set a response code?

[wayneashleyberry]

Hey @jehiah, I see you added a lot of tests for the compress handler (a21e9de) - do you perhaps have some insight into this?

[elithrar]

@wayneashleyberry The gzip middleware writes the header as it's own WriteHeader implementation deletes the Content-Length header, so it still needs to write the header the caller (application) requested.

If you comment that out, are you seeing the correct status codes written in all cases (e.g. non-200's)?

This should not have changed under Go 1.7, either.

[wayneashleyberry]

Ah no, if I comment out w.ResponseWriter.WriteHeader(c) then my 404's return 200 :(

[wayneashleyberry]

I'm actually running into the same issue using https://github.com/phyber/negroni-gzip instead.

[elithrar]

I suspect that https://github.com/NYTimes/gziphandler won't cause the issue

• I'd be curious if go install -gcflags=-ssa=0 worked as well, though.

On Mon, Sep 5, 2016 at 10:33 AM Wayne Ashley Berry notifications@github.com
wrote:

I'm actually running into the same issue using
https://github.com/phyber/negroni-gzip instead.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#83 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABIcN7580nPDsM_77Qrung5JvUoQLJUks5qnFJrgaJpZM4J0TWh
.

[wayneashleyberry]

Looks like I'm getting the exact same issue with the NYTimes/gziphandler, this is bleak :/

[wayneashleyberry]

Might be related to nytimes/gziphandler#5

[elithrar]

And with SSA disabled?

On Mon, Sep 5, 2016 at 11:21 AM Wayne Ashley Berry notifications@github.com
wrote:

Might be related to nytimes/gziphandler#5
nytimes/gziphandler#5?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#83 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABIcMMe4TQ87PUgeUhI5KhVwQIxfPkgks5qnF3AgaJpZM4J0TWh
.

[wayneashleyberry]

Not quite sure where I run that command @elithrar, I tried running it in $GOPATH/src/github.com/wayneashleyberry/go-website and then ran go build again but got the same issue.

[wayneashleyberry]

While checking out the source for http.Error I think I might've found something. For some reason, setting the content type header makes this problem go away. Does this make sense?

https://golang.org/src/net/http/server.go?s=51433:51485#L1725

package main

import (
    "fmt"
    "net/http"
    "os"

    "github.com/gorilla/handlers"
    "github.com/gorilla/mux"
)

func main() {
    r := mux.NewRouter()
    r.NotFoundHandler = http.HandlerFunc(NotFoundHandler)
    r.HandleFunc("/", HomeHandler)
    gzip := handlers.CompressHandler(r)
    http.ListenAndServe(":"+os.Getenv("PORT"), gzip)
}

func NotFoundHandler(w http.ResponseWriter, r *http.Request) {
    // A Content-Type header has to be set before calling w.WriteHeader,
    // otherwise WriteHeader is called twice (from this handler and
    // the compression handler) and the response breaks.
    w.Header().Set("Content-Type", "text/html; charset=utf-8")
    w.WriteHeader(http.StatusNotFound)
    fmt.Fprintf(w, "404 - not found")
}

func HomeHandler(w http.ResponseWriter, r *http.Request) {
    fmt.Fprintf(w, "Hello, World!")
}

[wayneashleyberry]

Setting the content-type header prevents the same thing from happening when using github.com/NYTimes/gziphandler so I'm gonna assume it'll work for the negroni middlewares as well. Although it works I'd love to know why and if this should even be considered a "fix".

[elithrar]

Without a Content-Type the browser doesn't know what to do. We should either:

• Document the requirement for Content-Type
• Set it as a last-resort ourselves.

Would you be able to PR this?

[jehiah]

@elithrar I think i can add some context here. For debugging curl --compressed -v ... is helpful.

Typically if Content-Type is omitted it's guessed by http.Server. quoting http.ResponseWriter.Write

If WriteHeader has not yet been called, Write calls
WriteHeader(http.StatusOK) before writing the data. If the Header
does not contain a Content-Type line, Write adds a Content-Type set
to the result of passing the initial 512 bytes of written data to
DetectContentType.

What is important is to make sure that Write() from http.Server is not guessing the content type as application/octet-stream which happens when the first 512 bytes are compressed. To avoid that handler.compressResponseWriter.Write correctly has content type detection logic so it's based on uncompressed content.

Unfortunately the docs for http.ResponseWriter.Write have an important context...

Changing the header after a call to WriteHeader (or Write) has no effect ...

So. handler.compressResponseWriter.Write can no longer modify headers after WriteHeader() is called because that flushes the headers of the request. This means that in non-200 code responses or any time WriteHeader is called before Write we should

a) skip compression
b) skip compression if we don't already have a content-type set
c) document/require a content-type must be set.

So if you put these things together, given the example from @wayneashleyberry you get Content-Type: application/octet-stream

curl -v --compressed 'http://127.0.0.1:8081/tess'
*   Trying 127.0.0.1...
* Connected to 127.0.0.1 (127.0.0.1) port 8081 (#0)
> GET /tess HTTP/1.1
> Host: 127.0.0.1:8081
> User-Agent: curl/7.43.0
> Accept: */*
> Accept-Encoding: deflate, gzip
> 
< HTTP/1.1 404 Not Found
< Content-Encoding: deflate
< Vary: Accept-Encoding
< Date: Sun, 18 Sep 2016 19:49:42 GMT
< Content-Length: 21
< Content-Type: application/octet-stream
< 
* Connection #0 to host 127.0.0.1 left intact
404 - Not Found

[elithrar]

Correct.

We need to effectively replicate wroteHeader in the default ResponseWriter
implementation - https://golang.org/src/net/http/server.go?s=2530:4338#L878 -
so that we can determine whether WriteHeader was called or not, and be more
intelligent around that.

On Sun, Sep 18, 2016 at 5:48 PM Jehiah Czebotar notifications@github.com
wrote:

@elithrar https://github.com/elithrar I think i can add some context
here. For debugging curl --compressed -v ... is helpful.

Typically if Content-Type is omitted it's guessed by http.Server. quoting
http.ResponseWriter.Write
https://golang.org/pkg/net/http/#ResponseWriter

If WriteHeader has not yet been called, Write calls
WriteHeader(http.StatusOK) before writing the data. If the Header
does not contain a Content-Type line, Write adds a Content-Type set
to the result of passing the initial 512 bytes of written data to
DetectContentType.

What is important is to make sure that Write() from http.Server is not
guessing the content type as application/octet-stream which happens when
the first 512 bytes are compressed. To avoid that
handler.CompressHandler.Write also has content type detection logic so
it's based on uncompressed content.

Unfortunately the docs for http.ResponseWriter.Write
https://golang.org/pkg/net/http/#ResponseWriter have an important
context...

Changing the header after a call to WriteHeader (or Write) has no effect
...

So. handler.CompressHandler.Write can no longer modify headers after
WriteHeader() is called because that flushes the headers of the request.
This means that in non-200 code responses or any time WriteHeader is called
before Write we should

a) skip compression
b) skip compression if we don't already have a content-type set
c) document/require a content-type must be set.

So if you put these things together, given the example from
@wayneashleyberry https://github.com/wayneashleyberry you get Content-Type:
application/octet-stream

curl -v --compressed 'http://127.0.0.1:8081/tess'

• Trying 127.0.0.1...
• Connected to 127.0.0.1 (127.0.0.1) port 8081 (#0)

  GET /tess HTTP/1.1
  Host: 127.0.0.1:8081
  User-Agent: curl/7.43.0
  Accept: /
  Accept-Encoding: deflate, gzip

  < HTTP/1.1 404 Not Found
  < Content-Encoding: deflate
  < Vary: Accept-Encoding
  < Date: Sun, 18 Sep 2016 19:49:42 GMT
  < Content-Length: 21
  < Content-Type: application/octet-stream
  <
• Connection #0 to host 127.0.0.1 left intact
  404 - Not Found

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#83 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABIcHRt8s9xTISSZMFjXcxp6Y3pm-ybks5qrdvtgaJpZM4J0TWh
.