Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http/httputil: ReverseProxy does not close response body if ModifyResponse returns an error #22658

Closed
edanbe opened this issue Nov 10, 2017 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@edanbe
Copy link
Contributor

edanbe commented Nov 10, 2017

What version of Go are you using (go version)?

go version go1.9.2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/user"
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build111677926=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Return an error from ReverseProxy's ModifyResponse function.
Test: https://play.golang.org/p/8N8zeMvox5

What did you expect to see?

ServeHTTP should close the response body before returning.

What did you see instead?

ServeHTTP does not defer the call to close the response body, in order to populate response trailers.
If ModifyResponse returns an error, ServeHTTP returns before closing the response body.

I'll prepare a CL if desired.

@odeke-em
Copy link
Member

Thank you @edanbe for the detailed report and for volunteering a fix. Please do so.

@bradfitz am going to mark this as a Go1.10 bug, as it seems like a simple fix and non-invasive, please advise if otherwise.

@odeke-em odeke-em added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 10, 2017
@odeke-em odeke-em added this to the Go1.10 milestone Nov 10, 2017
@gopherbot
Copy link

Change https://golang.org/cl/77170 mentions this issue: net/http/httputil: make ReverseProxy close response body if ModifyResponse returns an error

@odeke-em
Copy link
Member

Thank you very much @edanbe for the CL, I've added some feedback for a test on the CL, please take a look.

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Nov 14, 2017
…d requests

Previously when RoundTrip returned a non-nil error, the proxy returned a
StatusBadGateway error, instead of first calling ModifyResponse. This
commit first calls ModifyResponse, whether or not the error returned
from RoundTrip is nil.

Also closes response body when ModifyResponse returns an error. See #22658.

Fixes #21255

Change-Id: I5b5bf23a69ae5608f87d4ece756a1b4985ccaa9c
Reviewed-on: https://go-review.googlesource.com/54030
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Nov 14, 2018
@rsc rsc unassigned odeke-em and edanbe Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants