From d8cc4cb3ef5bff86cdde8c8c5b0a24c8e9180e91 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 16 Sep 2023 18:57:50 +0700 Subject: [PATCH] http3: introduce an HTTP/3 error type (#4039) * http3: introduce an HTTP/3 error type * http3: use a pointer receiver for the Error --- http3/body.go | 5 +-- http3/client.go | 4 +-- http3/error.go | 58 ++++++++++++++++++++++++++++++ http3/error_codes.go | 10 +++++- http3/error_test.go | 41 +++++++++++++++++++++ http3/response_writer.go | 7 ++-- integrationtests/self/http_test.go | 12 ++++--- 7 files changed, 125 insertions(+), 12 deletions(-) create mode 100644 http3/error.go create mode 100644 http3/error_test.go diff --git a/http3/body.go b/http3/body.go index 63ff4366..dc168e9e 100644 --- a/http3/body.go +++ b/http3/body.go @@ -63,7 +63,8 @@ func (r *body) wasStreamHijacked() bool { } func (r *body) Read(b []byte) (int, error) { - return r.str.Read(b) + n, err := r.str.Read(b) + return n, maybeReplaceError(err) } func (r *body) Close() error { @@ -106,7 +107,7 @@ func (r *hijackableBody) Read(b []byte) (int, error) { if err != nil { r.requestDone() } - return n, err + return n, maybeReplaceError(err) } func (r *hijackableBody) requestDone() { diff --git a/http3/client.go b/http3/client.go index cc2400e2..8aca4807 100644 --- a/http3/client.go +++ b/http3/client.go @@ -318,13 +318,13 @@ func (c *client) RoundTripOpt(req *http.Request, opt RoundTripOpt) (*http.Respon } conn.CloseWithError(quic.ApplicationErrorCode(rerr.connErr), reason) } - return nil, rerr.err + return nil, maybeReplaceError(rerr.err) } if opt.DontCloseRequestStream { close(reqDone) <-done } - return rsp, rerr.err + return rsp, maybeReplaceError(rerr.err) } // cancelingReader reads from the io.Reader. diff --git a/http3/error.go b/http3/error.go new file mode 100644 index 00000000..b96ebeec --- /dev/null +++ b/http3/error.go @@ -0,0 +1,58 @@ +package http3 + +import ( + "errors" + "fmt" + + "github.com/quic-go/quic-go" +) + +// Error is returned from the round tripper (for HTTP clients) +// and inside the HTTP handler (for HTTP servers) if an HTTP/3 error occurs. +// See section 8 of RFC 9114. +type Error struct { + Remote bool + ErrorCode ErrCode + ErrorMessage string +} + +var _ error = &Error{} + +func (e *Error) Error() string { + s := e.ErrorCode.string() + if s == "" { + s = fmt.Sprintf("H3 error (%#x)", uint64(e.ErrorCode)) + } + // Usually errors are remote. Only make it explicit for local errors. + if !e.Remote { + s += " (local)" + } + if e.ErrorMessage != "" { + s += ": " + e.ErrorMessage + } + return s +} + +func maybeReplaceError(err error) error { + if err == nil { + return nil + } + + var ( + e Error + strErr *quic.StreamError + appErr *quic.ApplicationError + ) + switch { + default: + return err + case errors.As(err, &strErr): + e.Remote = strErr.Remote + e.ErrorCode = ErrCode(strErr.ErrorCode) + case errors.As(err, &appErr): + e.Remote = appErr.Remote + e.ErrorCode = ErrCode(appErr.ErrorCode) + e.ErrorMessage = appErr.ErrorMessage + } + return &e +} diff --git a/http3/error_codes.go b/http3/error_codes.go index e8428d0a..ae646586 100644 --- a/http3/error_codes.go +++ b/http3/error_codes.go @@ -30,6 +30,14 @@ const ( ) func (e ErrCode) String() string { + s := e.string() + if s != "" { + return s + } + return fmt.Sprintf("unknown error code: %#x", uint16(e)) +} + +func (e ErrCode) string() string { switch e { case ErrCodeNoError: return "H3_NO_ERROR" @@ -68,6 +76,6 @@ func (e ErrCode) String() string { case ErrCodeDatagramError: return "H3_DATAGRAM_ERROR" default: - return fmt.Sprintf("unknown error code: %#x", uint16(e)) + return "" } } diff --git a/http3/error_test.go b/http3/error_test.go new file mode 100644 index 00000000..71567128 --- /dev/null +++ b/http3/error_test.go @@ -0,0 +1,41 @@ +package http3 + +import ( + "errors" + + "github.com/quic-go/quic-go" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("HTTP/3 errors", func() { + It("converts", func() { + Expect(maybeReplaceError(nil)).To(BeNil()) + Expect(maybeReplaceError(errors.New("foobar"))).To(MatchError("foobar")) + Expect(maybeReplaceError(&quic.StreamError{ + ErrorCode: 1337, + Remote: true, + })).To(Equal(&Error{ + Remote: true, + ErrorCode: 1337, + })) + Expect(maybeReplaceError(&quic.ApplicationError{ + ErrorCode: 42, + Remote: true, + ErrorMessage: "foobar", + })).To(Equal(&Error{ + Remote: true, + ErrorCode: 42, + ErrorMessage: "foobar", + })) + }) + + It("has a string representation", func() { + Expect((&Error{ErrorCode: 0x10c, Remote: true}).Error()).To(Equal("H3_REQUEST_CANCELLED")) + Expect((&Error{ErrorCode: 0x10c, Remote: true, ErrorMessage: "foobar"}).Error()).To(Equal("H3_REQUEST_CANCELLED: foobar")) + Expect((&Error{ErrorCode: 0x10c, Remote: false}).Error()).To(Equal("H3_REQUEST_CANCELLED (local)")) + Expect((&Error{ErrorCode: 0x10c, Remote: false, ErrorMessage: "foobar"}).Error()).To(Equal("H3_REQUEST_CANCELLED (local): foobar")) + Expect((&Error{ErrorCode: 0x1337, Remote: true}).Error()).To(Equal("H3 error (0x1337)")) + }) +}) diff --git a/http3/response_writer.go b/http3/response_writer.go index 90a30497..0d931478 100644 --- a/http3/response_writer.go +++ b/http3/response_writer.go @@ -166,9 +166,10 @@ func (w *responseWriter) Write(p []byte) (int, error) { w.buf = w.buf[:0] w.buf = df.Append(w.buf) if _, err := w.bufferedStr.Write(w.buf); err != nil { - return 0, err + return 0, maybeReplaceError(err) } - return w.bufferedStr.Write(p) + n, err := w.bufferedStr.Write(p) + return n, maybeReplaceError(err) } func (w *responseWriter) FlushError() error { @@ -177,7 +178,7 @@ func (w *responseWriter) FlushError() error { } if !w.written { if err := w.writeHeader(); err != nil { - return err + return maybeReplaceError(err) } w.written = true } diff --git a/integrationtests/self/http_test.go b/integrationtests/self/http_test.go index a1067070..ac16ffa5 100644 --- a/integrationtests/self/http_test.go +++ b/integrationtests/self/http_test.go @@ -307,9 +307,10 @@ var _ = Describe("HTTP tests", func() { for { if _, err := w.Write([]byte("foobar")); err != nil { Expect(r.Context().Done()).To(BeClosed()) - var strErr *quic.StreamError - Expect(errors.As(err, &strErr)).To(BeTrue()) - Expect(strErr.ErrorCode).To(Equal(quic.StreamErrorCode(0x10c))) + var http3Err *http3.Error + Expect(errors.As(err, &http3Err)).To(BeTrue()) + Expect(http3Err.ErrorCode).To(Equal(http3.ErrCode(0x10c))) + Expect(http3Err.Error()).To(Equal("H3_REQUEST_CANCELLED")) return } } @@ -325,7 +326,10 @@ var _ = Describe("HTTP tests", func() { cancel() Eventually(handlerCalled).Should(BeClosed()) _, err = resp.Body.Read([]byte{0}) - Expect(err).To(HaveOccurred()) + var http3Err *http3.Error + Expect(errors.As(err, &http3Err)).To(BeTrue()) + Expect(http3Err.ErrorCode).To(Equal(http3.ErrCode(0x10c))) + Expect(http3Err.Error()).To(Equal("H3_REQUEST_CANCELLED (local)")) }) It("allows streamed HTTP requests", func() {