[go] net/http/httputil: don't panic in ReverseProxy unless running under a Server
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick would like Ian Lance Taylor and Andrew Bonventre to review this change.
net/http/httputil: don't panic in ReverseProxy unless running under a Server
Prior to the fix to #23643, the ReverseProxy didn't panic with
ErrAbortHandler when the copy to a client failed.
During Go 1.11 beta testing, we found plenty of code using
ReverseProxy in tests that were unprepared for a panic.
Change the behavior to only panic when running under the http.Server
that'll handle the panic.
Updates #23643
Change-Id: Ic1fa8405fd54c858ce8c797cec79d006833a9f7d
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 2eda6b7..79dfa32 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -253,7 +253,11 @@
defer res.Body.Close()
// Since we're streaming the response, if we run into an error all we can do
// is abort the request. Issue 23643: ReverseProxy should use ErrAbortHandler
- // on read error while copying body
+ // on read error while copying body.
+ if !shouldPanicOnCopyError(req) {
+ log.Printf("suppressing panic for copyResponse error in test; copy error: %v", err)
+ return
+ }
panic(http.ErrAbortHandler)
}
res.Body.Close() // close now, instead of defer, to populate res.Trailer
@@ -271,6 +275,29 @@
}
}
+var inOurTests bool // whether we're in our own tests
+
+// shouldPanicOnCopyError reports whether the reverse proxy should
+// panic with http.ErrAbortHandler. This is the right thing to do by
+// default, but Go 1.10 and earlier did not, so existing unit tests
+// weren't expecting panics. Only panic in our own tests, or when
+// running under the HTTP server.
+// under an HTTP server.
+func shouldPanicOnCopyError(req *http.Request) bool {
+ if inOurTests {
+ // Our tests know to handle this panic.
+ return true
+ }
+ if req.Context().Value(http.ServerContextKey) != nil {
+ // We seem to be running under an HTTP server, so
+ // it'll recover the panic.
+ return true
+ }
+ // Otherwise act like Go 1.10 and earlier to not break
+ // existing tests.
+ return false
+}
+
// removeConnectionHeaders removes hop-by-hop headers listed in the "Connection" header of h.
// See RFC 7230, section 6.1
func removeConnectionHeaders(h http.Header) {
diff --git a/src/net/http/httputil/reverseproxy_test.go b/src/net/http/httputil/reverseproxy_test.go
index 0240bfa..2a12e75 100644
--- a/src/net/http/httputil/reverseproxy_test.go
+++ b/src/net/http/httputil/reverseproxy_test.go
@@ -29,6 +29,7 @@
const fakeHopHeader = "X-Fake-Hop-Header-For-Test"
func init() {
+ inOurTests = true
hopHeaders = append(hopHeaders, fakeHopHeader)
}
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Patch set 1:Code-Review +2
1 comment:
File src/net/http/httputil/reverseproxy.go:
Patch Set #1, Line 285:
// under an HTTP server.Delete this line.
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Uploaded patch set 2.
(2 comments)
2 comments:
File src/net/http/httputil/reverseproxy.go:
Patch Set #1, Line 258:
log.Printf("suppressing panic for copyResponse error in test; copy error: %v", err)Is there a way to suppress this logging? (In our prod servers, this will be very common, occurring w […]
This won't log in production. This is only for the path where the panic is skipped for Go 1.10 test compatibility.
Patch Set #1, Line 285:
func shouldPanicOnCopyError(req *http.Request) bool {Delete this line.
Done
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick uploaded patch set #2 to this change.
net/http/httputil: don't panic in ReverseProxy unless running under a Server
Prior to the fix to #23643, the ReverseProxy didn't panic with
ErrAbortHandler when the copy to a client failed.
During Go 1.11 beta testing, we found plenty of code using
ReverseProxy in tests that were unprepared for a panic.
Change the behavior to only panic when running under the http.Server
that'll handle the panic.
Updates #23643
Change-Id: Ic1fa8405fd54c858ce8c797cec79d006833a9f7d
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 28 insertions(+), 1 deletion(-)
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Patch set 2:Run-TryBot +1
Gobot Gobot (Gerrit)
Michael Piatek (Gerrit)
1 comment:
Patch Set #1, Line 258:
log.Printf("suppressing panic for copyResponse error in test; copy error: %v", err)
Is there a way to suppress this logging? (In our prod servers, this will be very common, occurring when users close their browser window.)
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 2:TryBot-Result +1
Michael Piatek (Gerrit)
1 comment:
File src/net/http/httputil/reverseproxy.go:
Patch Set #1, Line 258:
log.Printf("suppressing panic for copyResponse error in test; copy error: %v", err)This won't log in production. This is only for the path where the panic is skipped for Go 1. […]
I might be missing something, but I think we'll up here on our prod code paths. (The panic motivating this change occurs from a call to ServeHTTP outside the stack of an http.Server, so !shouldPanicOnCopyError and this logging IIUC.)
Feel free to ping me on chat if you'd like to discuss directly.
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Uploaded patch set 3.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick uploaded patch set #3 to this change.
net/http/httputil: don't panic in ReverseProxy unless running under a Server
Prior to the fix to #23643, the ReverseProxy didn't panic with
ErrAbortHandler when the copy to a client failed.
During Go 1.11 beta testing, we found plenty of code using
ReverseProxy in tests that were unprepared for a panic.
Change the behavior to only panic when running under the http.Server
that'll handle the panic.
Updates #23643
Change-Id: Ic1fa8405fd54c858ce8c797cec79d006833a9f7d
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 28 insertions(+), 1 deletion(-)
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
1 comment:
File src/net/http/httputil/reverseproxy.go:
Patch Set #1, Line 258:
p.logf("suppressing panic for copyResponse error in test; copy error: %v", err)I might be missing something, but I think we'll up here on our prod code paths. […]
Per chat, you'll need to handle ErrAbortHandler yourself if you have an alternate server implementation.
To view, visit change 122819. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Brad Fitzpatrick merged this change.
net/http/httputil: don't panic in ReverseProxy unless running under a Server
Prior to the fix to #23643, the ReverseProxy didn't panic with
ErrAbortHandler when the copy to a client failed.
During Go 1.11 beta testing, we found plenty of code using
ReverseProxy in tests that were unprepared for a panic.
Change the behavior to only panic when running under the http.Server
that'll handle the panic.
Updates #23643
Change-Id: Ic1fa8405fd54c858ce8c797cec79d006833a9f7d
Reviewed-on: https://go-review.googlesource.com/122819
Reviewed-by: Ian Lance Taylor <ia...@golang.org>
---
M src/net/http/httputil/reverseproxy.go
M src/net/http/httputil/reverseproxy_test.go
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/src/net/http/httputil/reverseproxy.go b/src/net/http/httputil/reverseproxy.go
index 2eda6b7..6f0a241 100644
--- a/src/net/http/httputil/reverseproxy.go
+++ b/src/net/http/httputil/reverseproxy.go
@@ -253,7 +253,11 @@
defer res.Body.Close()
// Since we're streaming the response, if we run into an error all we can do
// is abort the request. Issue 23643: ReverseProxy should use ErrAbortHandler
- // on read error while copying body
+ // on read error while copying body.
+ if !shouldPanicOnCopyError(req) {
+ p.logf("suppressing panic for copyResponse error in test; copy error: %v", err)
+ return
+ }
panic(http.ErrAbortHandler)
}
res.Body.Close() // close now, instead of defer, to populate res.Trailer
@@ -271,6 +275,28 @@
}
}
+var inOurTests bool // whether we're in our own tests
+
+// shouldPanicOnCopyError reports whether the reverse proxy should
+// panic with http.ErrAbortHandler. This is the right thing to do by
+// default, but Go 1.10 and earlier did not, so existing unit tests
+// weren't expecting panics. Only panic in our own tests, or when
+// running under the HTTP server.