From fc4adb47755865c5cd1391b01bc44862abd854b2 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 30 Dec 2017 13:00:34 +0700 Subject: [PATCH] improve error handling in the h2quic client for header stream handling When the underlying QUIC stream is closed, the close error should be returned. This always happens when receiving a CONNECTION_CLOSE from the server. Furthermore, this adds a missing break statement in the case when receiving an invalid HTTP request. --- h2quic/client.go | 23 +++++++------- h2quic/client_test.go | 71 +++++++++++++++++++------------------------ 2 files changed, 44 insertions(+), 50 deletions(-) diff --git a/h2quic/client.go b/h2quic/client.go index 9d845ec0..b0b35bce 100644 --- a/h2quic/client.go +++ b/h2quic/client.go @@ -97,24 +97,23 @@ func (c *client) handleHeaderStream() { decoder := hpack.NewDecoder(4096, func(hf hpack.HeaderField) {}) h2framer := http2.NewFramer(nil, c.headerStream) - var lastStream protocol.StreamID - + var err error for { - frame, err := h2framer.ReadFrame() + var frame http2.Frame + frame, err = h2framer.ReadFrame() if err != nil { - c.headerErr = qerr.Error(qerr.HeadersStreamDataDecompressFailure, "cannot read frame") break } - lastStream = protocol.StreamID(frame.Header().StreamID) + lastStream := protocol.StreamID(frame.Header().StreamID) hframe, ok := frame.(*http2.HeadersFrame) if !ok { - c.headerErr = qerr.Error(qerr.InvalidHeadersStreamData, "not a headers frame") + err = errors.New("not a headers frame") break } mhframe := &http2.MetaHeadersFrame{HeadersFrame: hframe} mhframe.Fields, err = decoder.DecodeFull(hframe.HeaderBlockFragment()) if err != nil { - c.headerErr = qerr.Error(qerr.InvalidHeadersStreamData, "cannot read header fields") + err = fmt.Errorf("cannot read header fields: %s", err.Error()) break } @@ -122,19 +121,21 @@ func (c *client) handleHeaderStream() { responseChan, ok := c.responses[protocol.StreamID(hframe.StreamID)] c.mutex.RUnlock() if !ok { - c.headerErr = qerr.Error(qerr.InternalError, fmt.Sprintf("h2client BUG: response channel for stream %d not found", lastStream)) + err = fmt.Errorf("response channel for stream %d not found", lastStream) break } - rsp, err := responseFromHeaders(mhframe) + var rsp *http.Response + rsp, err = responseFromHeaders(mhframe) if err != nil { - c.headerErr = qerr.Error(qerr.InternalError, err.Error()) + break } responseChan <- rsp } + utils.Debugf("Error handling header stream: %s", err) + c.headerErr = qerr.Error(qerr.InvalidHeadersStreamData, err.Error()) // stop all running request - utils.Debugf("Error handling header stream %d: %s", lastStream, c.headerErr.Error()) close(c.headerErrored) } diff --git a/h2quic/client_test.go b/h2quic/client_test.go index 24737e19..49d9b6d8 100644 --- a/h2quic/client_test.go +++ b/h2quic/client_test.go @@ -188,41 +188,31 @@ var _ = Describe("Client", func() { close(done) }) - It("closes the quic client when encountering an error on the header stream", func(done Done) { + It("closes the quic client when encountering an error on the header stream", func() { headerStream.dataToRead.Write(bytes.Repeat([]byte{0}, 100)) - var doReturned bool + done := make(chan struct{}) go func() { defer GinkgoRecover() - var err error rsp, err := client.RoundTrip(request) Expect(err).To(MatchError(client.headerErr)) Expect(rsp).To(BeNil()) - doReturned = true + close(done) }() - Eventually(func() bool { return doReturned }).Should(BeTrue()) - Expect(client.headerErr).To(MatchError(qerr.Error(qerr.HeadersStreamDataDecompressFailure, "cannot read frame"))) + Eventually(done).Should(BeClosed()) + Expect(client.headerErr.ErrorCode).To(Equal(qerr.InvalidHeadersStreamData)) Expect(client.session.(*mockSession).closedWithError).To(MatchError(client.headerErr)) - close(done) - }, 2) + }) - It("returns subsequent request if there was an error on the header stream before", func(done Done) { - expectedErr := qerr.Error(qerr.HeadersStreamDataDecompressFailure, "cannot read frame") + It("returns subsequent request if there was an error on the header stream before", func() { session.streamsToOpen = []quic.Stream{headerStream, dataStream, newMockStream(7)} headerStream.dataToRead.Write(bytes.Repeat([]byte{0}, 100)) - var firstReqReturned bool - go func() { - defer GinkgoRecover() - _, err := client.RoundTrip(request) - Expect(err).To(MatchError(expectedErr)) - firstReqReturned = true - }() - - Eventually(func() bool { return firstReqReturned }).Should(BeTrue()) - // now that the first request failed due to an error on the header stream, try another request _, err := client.RoundTrip(request) - Expect(err).To(MatchError(expectedErr)) - close(done) + Expect(err).To(BeAssignableToTypeOf(&qerr.QuicError{})) + Expect(err.(*qerr.QuicError).ErrorCode).To(Equal(qerr.InvalidHeadersStreamData)) + // now that the first request failed due to an error on the header stream, try another request + _, nextErr := client.RoundTrip(request) + Expect(nextErr).To(MatchError(err)) }) It("blocks if no stream is available", func() { @@ -479,16 +469,9 @@ var _ = Describe("Client", func() { It("errors if the H2 frame is not a HeadersFrame", func() { h2framer.WritePing(true, [8]byte{0, 0, 0, 0, 0, 0, 0, 0}) - - var handlerReturned bool - go func() { - client.handleHeaderStream() - handlerReturned = true - }() - + client.handleHeaderStream() Eventually(client.headerErrored).Should(BeClosed()) Expect(client.headerErr).To(MatchError(qerr.Error(qerr.InvalidHeadersStreamData, "not a headers frame"))) - Eventually(func() bool { return handlerReturned }).Should(BeTrue()) }) It("errors if it can't read the HPACK encoded header fields", func() { @@ -497,16 +480,26 @@ var _ = Describe("Client", func() { EndHeaders: true, BlockFragment: []byte("invalid HPACK data"), }) - - var handlerReturned bool - go func() { - client.handleHeaderStream() - handlerReturned = true - }() - + client.handleHeaderStream() Eventually(client.headerErrored).Should(BeClosed()) - Expect(client.headerErr).To(MatchError(qerr.Error(qerr.InvalidHeadersStreamData, "cannot read header fields"))) - Eventually(func() bool { return handlerReturned }).Should(BeTrue()) + Expect(client.headerErr.ErrorCode).To(Equal(qerr.InvalidHeadersStreamData)) + Expect(client.headerErr.ErrorMessage).To(ContainSubstring("cannot read header fields")) + }) + + It("errors if the stream cannot be found", func() { + var headers bytes.Buffer + enc := hpack.NewEncoder(&headers) + enc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"}) + err := h2framer.WriteHeaders(http2.HeadersFrameParam{ + StreamID: 1337, + EndHeaders: true, + BlockFragment: headers.Bytes(), + }) + Expect(err).ToNot(HaveOccurred()) + client.handleHeaderStream() + Eventually(client.headerErrored).Should(BeClosed()) + Expect(client.headerErr.ErrorCode).To(Equal(qerr.InvalidHeadersStreamData)) + Expect(client.headerErr.ErrorMessage).To(ContainSubstring("response channel for stream 1337 not found")) }) }) })