From 5eb236b6cedc95e4911263ccacc93e34bea104b0 Mon Sep 17 00:00:00 2001 From: Shade Alabsa Date: Fri, 7 Oct 2022 13:25:30 -0400 Subject: [PATCH 1/6] Handle abort like the stdlib does --- http3/server.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/http3/server.go b/http3/server.go index b0204416781..5058e5bae52 100644 --- a/http3/server.go +++ b/http3/server.go @@ -42,6 +42,12 @@ const ( streamTypeQPACKDecoderStream = 3 ) +// ErrAbortHandler is a sentinel panic value to abort a handler. +// While any panic from ServeHTTP aborts the response to the client, +// panicking with ErrAbortHandler also suppresses logging of a stack +// trace to the server's error log. +var ErrAbortHandler = errors.New("net/http: abort Handler") + func versionToALPN(v protocol.VersionNumber) string { if v == protocol.Version1 || v == protocol.Version2 { return nextProtoH3 @@ -575,7 +581,7 @@ func (s *Server) handleRequest(conn quic.Connection, str quic.Stream, decoder *q var panicked bool func() { defer func() { - if p := recover(); p != nil { + if p := recover(); p != nil && err != ErrAbortHandler { // Copied from net/http/server.go const size = 64 << 10 buf := make([]byte, size) From d148d8c8c26db35f75bb36277641c091739af417 Mon Sep 17 00:00:00 2001 From: Shade Alabsa Date: Fri, 7 Oct 2022 15:01:47 -0400 Subject: [PATCH 2/6] Using the sentinel value from the stdlib instead of redefining. --- http3/server.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/http3/server.go b/http3/server.go index 5058e5bae52..fa9292f855c 100644 --- a/http3/server.go +++ b/http3/server.go @@ -42,12 +42,6 @@ const ( streamTypeQPACKDecoderStream = 3 ) -// ErrAbortHandler is a sentinel panic value to abort a handler. -// While any panic from ServeHTTP aborts the response to the client, -// panicking with ErrAbortHandler also suppresses logging of a stack -// trace to the server's error log. -var ErrAbortHandler = errors.New("net/http: abort Handler") - func versionToALPN(v protocol.VersionNumber) string { if v == protocol.Version1 || v == protocol.Version2 { return nextProtoH3 @@ -581,7 +575,7 @@ func (s *Server) handleRequest(conn quic.Connection, str quic.Stream, decoder *q var panicked bool func() { defer func() { - if p := recover(); p != nil && err != ErrAbortHandler { + if p := recover(); p != nil && err != http.ErrAbortHandler { // Copied from net/http/server.go const size = 64 << 10 buf := make([]byte, size) From 15031b1821ce7bcc28f8455456de4b1e14a94629 Mon Sep 17 00:00:00 2001 From: Shade Alabsa Date: Fri, 7 Oct 2022 21:22:52 -0400 Subject: [PATCH 3/6] we return before logging out --- http3/server.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/http3/server.go b/http3/server.go index fa9292f855c..c27d8286b2f 100644 --- a/http3/server.go +++ b/http3/server.go @@ -575,13 +575,16 @@ func (s *Server) handleRequest(conn quic.Connection, str quic.Stream, decoder *q var panicked bool func() { defer func() { - if p := recover(); p != nil && err != http.ErrAbortHandler { + if p := recover(); p != nil { + panicked = true + if p == http.ErrAbortHandler { + return + } // Copied from net/http/server.go const size = 64 << 10 buf := make([]byte, size) buf = buf[:runtime.Stack(buf, false)] s.logger.Errorf("http: panic serving: %v\n%s", p, buf) - panicked = true } }() handler.ServeHTTP(r, req) From c5501cf383848ac51f7d638fb3594ad8960667d6 Mon Sep 17 00:00:00 2001 From: Shade Alabsa Date: Sat, 8 Oct 2022 08:20:40 -0400 Subject: [PATCH 4/6] Added test to hand abort handler --- http3/server_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/http3/server_test.go b/http3/server_test.go index 9008067e8a2..36894c1d02c 100644 --- a/http3/server_test.go +++ b/http3/server_test.go @@ -201,6 +201,19 @@ var _ = Describe("Server", func() { Expect(hfs).To(HaveKeyWithValue(":status", []string{"200"})) }) + It("handles aborts", func() { + testErr := http.ErrAbortHandler + done := make(chan struct{}) + unknownStr := mockquic.NewMockStream(mockCtrl) + s.UniStreamHijacker = func(st StreamType, _ quic.Connection, str quic.ReceiveStream, err error) bool { + defer close(done) + Expect(st).To(BeZero()) + Expect(str).To(Equal(unknownStr)) + Expect(err).To(MatchError(testErr)) + return true + } + }) + It("handles a panicking handler", func() { s.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { panic("foobar") From a5091d0afefd3512e1811c9e51da3db760b881fa Mon Sep 17 00:00:00 2001 From: Shade Alabsa Date: Sat, 8 Oct 2022 08:28:41 -0400 Subject: [PATCH 5/6] Added in two tests but apparently only saved the first one. --- http3/server_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/http3/server_test.go b/http3/server_test.go index 36894c1d02c..1156e5bb7e8 100644 --- a/http3/server_test.go +++ b/http3/server_test.go @@ -214,6 +214,23 @@ var _ = Describe("Server", func() { } }) + It("handles a aborting handler", func() { + s.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + panic(http.ErrAbortHandler) + }) + + responseBuf := &bytes.Buffer{} + setRequest(encodeRequest(exampleGetRequest)) + str.EXPECT().Context().Return(reqContext) + str.EXPECT().Write(gomock.Any()).DoAndReturn(responseBuf.Write).AnyTimes() + str.EXPECT().CancelRead(gomock.Any()) + + serr := s.handleRequest(conn, str, qpackDecoder, nil) + Expect(serr.err).ToNot(HaveOccurred()) + hfs := decodeHeader(responseBuf) + Expect(hfs).To(HaveKeyWithValue(":status", []string{"500"})) + }) + It("handles a panicking handler", func() { s.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { panic("foobar") From 0dd041afbda8762db14a5385b68e8dddf1b900d5 Mon Sep 17 00:00:00 2001 From: Shade Alabsa Date: Sun, 9 Oct 2022 07:00:48 -0400 Subject: [PATCH 6/6] remove one test case because it wasn't needed --- http3/server_test.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/http3/server_test.go b/http3/server_test.go index 1156e5bb7e8..15c59cc611c 100644 --- a/http3/server_test.go +++ b/http3/server_test.go @@ -201,19 +201,6 @@ var _ = Describe("Server", func() { Expect(hfs).To(HaveKeyWithValue(":status", []string{"200"})) }) - It("handles aborts", func() { - testErr := http.ErrAbortHandler - done := make(chan struct{}) - unknownStr := mockquic.NewMockStream(mockCtrl) - s.UniStreamHijacker = func(st StreamType, _ quic.Connection, str quic.ReceiveStream, err error) bool { - defer close(done) - Expect(st).To(BeZero()) - Expect(str).To(Equal(unknownStr)) - Expect(err).To(MatchError(testErr)) - return true - } - }) - It("handles a aborting handler", func() { s.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { panic(http.ErrAbortHandler)