Can't set response headers when implementing custom Recovery Formatter

[nicklaw5]

The docs indicate that when overriding the default Recovery.Formatter, it's possible to set response headers. I'm currently attempting to set the Content-Type header to application/json, however it appears that doesn't stick and reverts to text/plain; charset=utf-8.

Example:

package main

import (
    "fmt"
    "net/http"

    "github.com/urfave/negroni"
)

type PanicFormatter struct{}

func (t *PanicFormatter) FormatPanicError(rw http.ResponseWriter, r *http.Request, infos *negroni.PanicInformation) {
    rw.Header().Set("Content-Type", "application/json")
    fmt.Fprintf(rw, `{"error":"%s"}`, infos.RecoveredPanic)
}

func main() {
    mux := http.NewServeMux()
    mux.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
        panic("oh no!")
    })

    n := negroni.New()
    recovery := negroni.NewRecovery()
    recovery.Formatter = &PanicFormatter{}
    n.Use(recovery)
    n.UseHandler(mux)

    http.ListenAndServe(":3003", n)
}

$ curl -i localhost:3003
HTTP/1.1 500 Internal Server Error
Date: Fri, 16 Nov 2018 20:32:32 GMT
Content-Length: 18
Content-Type: text/plain; charset=utf-8

{"error":"oh no!"}

[jszwedko]

:+1: thanks for the report @nicklaw5

It looks like the ResponseWriter.WriteHeader() call flushes, disallowing any additional header setting.

My initial thought is to move that call into the default PanicFormatter and require library users overriding the Formatter to set their own response code. This would, however, make it more of a PanicHandler than a PanicFormatter so perhaps one solution is to introduce a new PanicHandler field and deprecate the `PanicFormatter. This would also allow us to preserve compatibility.

I'll think about this a bit more, but suggestions welcome!

[nicklaw5]

Your suggestion of introducing a PanicHandler and deprecating the PanicFormatter sounds good to me. If we could also preserve writing the default 500 Internal Server Error response status, unless ResponseWriter.WriteHeader() is called within PanicHandler, that would also be ideal.

[jszwedko]

:+1: agreed; I think that should be feasible by passing in a different ResponseWriter that would do something akin to https://github.com/urfave/negroni/blob/master/response_writer.go#L56-L64.

I'll try to get to this, but PRs also welcome :smile: