CC @bradfitz

@mdp To not break existing programs I would suggest to return the same header as before when no error handler is provided.

@mitch000001 Yeah, good point. I was trying to align this with #9864, which was marked a "go1.5maybe" and wanted to match it's return BadGateway(502), but it's probably better to keep that as a separate issue. Fixed in my proposed solution.

Want to send a change?
https://golang.org/doc/contribute.html

Done - https://go-review.googlesource.com/#/c/16839/

CL https://golang.org/cl/16839 mentions this issue.

This can also be solved without changing ReverseProxy:

type errorHandlingRoundTripper struct {
        tr http.RoundTripper
}

func (rt *errorHandlingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
        tr := errorHandlingRoundTripper.tr
        if tr == nil {
                tr = http.DefaultTransport
        }
        res, err := rt.tr.RoundTrip(req)
        if isBadGateway(err) {
                return &http.Response{
                        StatusCode: StatusBadGateway,
                        // more stuff
                }, nil
        }
        return res, err
}

And use like:

rt := &httputil.RoundTripper{Transport: errorHandlingRoundTripper{}}

Yeah, I mentioned this option at the end of opening issue. As you've shown, it's a trivial amount of code to implement without modifying ReverseProxy.

My arguments for:

• Having the TransportErrorHandler makes it clear in the docs how to handle errors.
• It's a fairly common use case to catch upstream errors and handle them with custom logic for downstream clients, so having a simple handler would make it clear to the developer that it's something they should probably handle.
• It doesn't break existing implementations.

But like you've shown, it's easy to work around without any modification to ReverseProxy. I'm happy to close the issue if that's what's determined. Thanks

I would prefer a customized transport for that in order to minimize API and keep "tuning knobs" away.

But if it is really common to do that, I would suggest to add the code from @bmizerany as an example to the package.

I might be the exception here, but if I'm building a reverse proxy I will always want to handle the error with a custom function rather than a blank response and a 500. For me it's less of a "tuning knob" and more of something that I will always have to implement as a custom transport.

But if I'm the exception then I agree, it's definitely an unnecessary addition to the API.

@bmizerany Thoughts? If we don't want to change ReverseProxy I think your example code would still be a nice addition to the documentation as @nightlyone suggested

I don't want to add something to the public API if there is a clean alternative already in existence, but that isn't my decision. I defer to @bradfitz and gang for more thoughts.

Given Blake and Ingo's points that this can be done already without changes, and given that we're trying to wind down work on the standard library, I think we'll pass on this change.

Somebody should probably fork this whole package and go nuts with it on github.