net/http/httputil: ReverseProxy change conflicts with future ReverseProxy plans

[rsc]

CL 54030 makes the suspicious change that after

res, err := roundtripper.RoundTrip(...)

the error condition is now detected by res == nil instead of err != nil. That seems wrong. Commented more on the CL itself but we should resolve this one way or the other for Go 1.10.

[rsc]

I'm also not even convinced the main part of the change is correct. The docs for the ModifyResponse field say:

// ModifyResponse is an optional function that
// modifies the Response from the backend.
// If it returns an error, the proxy returns a StatusBadGateway error.

The whole point of CL 54030 is to now call ModifyResponse on a fake Response that is not from the backend. Why is that correct?

/cc @bradfitz @tombergan

[smasher164]

Replied on the CL, but you're correct in that err != nil should be used to construct the fake Response.

As to why the fake res not from the backend is constructed, ModifyResponse needs some way of mutating the res when it is nil. I assumed that the correct default response to construct was one that returned a StatusBadGateway error.

The change was intended to adapt ModifyResponse's current API to support the case when err != nil. A more robust change for this would be to add an api like http://golang.org/issue/22700 which allows the ErrorHandler to take advantage of the error value.

[rsc]

It seems to me that CL 54030 and CL 77410 have different, conflicting ideas of how to process a RoundTrip error. If we think that CL 77410 may be the right choice in Go 1.11, then it doesn't make sense to include CL 54030 in Go 1.10.

Given the uncertainty around the API decision here I suggest we revert CL 54030 for Go 1.10 and decide the API question in the next round without having painted ourselves into a corner.

[smasher164]

I am fine with that, especially considering that the ErrorHandler in CL 77410 is only called when err != nil. That means that ModifyResponse is only invoked on a successful response.

[gopherbot]

Change https://golang.org/cl/86435 mentions this issue: Revert "net/http/httputil: allow ReverseProxy to call ModifyResponse on failed requests"

[gopherbot]

Change https://golang.org/cl/86476 mentions this issue: doc/go1.10: remove ReverseProxy TODO