From a472ac47314d49f943f56f8ef66d49b3520e4882 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 8 Nov 2019 13:46:20 +0700 Subject: [PATCH 1/2] use application-specific CONNECTION_CLOSE for application errors --- internal/qerr/quic_error.go | 6 ++++++ internal/qerr/quic_error_test.go | 7 ++++++- session.go | 7 ++++--- session_test.go | 13 +++++++++---- 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/internal/qerr/quic_error.go b/internal/qerr/quic_error.go index c4347afa2..66e8960c5 100644 --- a/internal/qerr/quic_error.go +++ b/internal/qerr/quic_error.go @@ -39,6 +39,7 @@ func CryptoError(tlsAlert uint8, errorMessage string) *QuicError { } } +// ApplicationError creates a new QuicError instance for an application error func ApplicationError(errorCode ErrorCode, errorMessage string) *QuicError { return &QuicError{ ErrorCode: errorCode, @@ -65,6 +66,11 @@ func (e *QuicError) IsCryptoError() bool { return e.ErrorCode.isCryptoError() } +// IsApplicationError says if this error is an application error +func (e *QuicError) IsApplicationError() bool { + return e.isApplicationError +} + // Temporary says if the error is temporary. func (e *QuicError) Temporary() bool { return false diff --git a/internal/qerr/quic_error_test.go b/internal/qerr/quic_error_test.go index eeeaac6ce..9c48487e3 100644 --- a/internal/qerr/quic_error_test.go +++ b/internal/qerr/quic_error_test.go @@ -11,6 +11,7 @@ var _ = Describe("QUIC Transport Errors", func() { It("has a string representation", func() { err := Error(FlowControlError, "foobar") Expect(err.Timeout()).To(BeFalse()) + Expect(err.IsApplicationError()).To(BeFalse()) Expect(err.Error()).To(Equal("FLOW_CONTROL_ERROR: foobar")) }) @@ -38,13 +39,17 @@ var _ = Describe("QUIC Transport Errors", func() { It("says if an error is a crypto error", func() { Expect(Error(FlowControlError, "").IsCryptoError()).To(BeFalse()) - Expect(CryptoError(42, "").IsCryptoError()).To(BeTrue()) + err := CryptoError(42, "") + Expect(err.IsCryptoError()).To(BeTrue()) + Expect(err.IsApplicationError()).To(BeFalse()) + }) }) Context("application errors", func() { It("has a string representation for errors with a message", func() { err := ApplicationError(0x42, "foobar") + Expect(err.IsApplicationError()).To(BeTrue()) Expect(err.Error()).To(Equal("Application error 0x42: foobar")) }) diff --git a/session.go b/session.go index 828ee09b9..5353bcecf 100644 --- a/session.go +++ b/session.go @@ -1023,7 +1023,7 @@ func (s *session) Close() error { } func (s *session) CloseWithError(code protocol.ApplicationErrorCode, desc string) error { - s.closeLocal(qerr.Error(qerr.ErrorCode(code), desc)) + s.closeLocal(qerr.ApplicationError(qerr.ErrorCode(code), desc)) <-s.ctx.Done() return nil } @@ -1250,8 +1250,9 @@ func (s *session) sendConnectionClose(quicErr *qerr.QuicError) ([]byte, error) { reason = quicErr.ErrorMessage } packet, err := s.packer.PackConnectionClose(&wire.ConnectionCloseFrame{ - ErrorCode: quicErr.ErrorCode, - ReasonPhrase: reason, + IsApplicationError: quicErr.IsApplicationError(), + ErrorCode: quicErr.ErrorCode, + ReasonPhrase: reason, }) if err != nil { return nil, err diff --git a/session_test.go b/session_test.go index 23da44340..ea3e05f25 100644 --- a/session_test.go +++ b/session_test.go @@ -449,12 +449,17 @@ var _ = Describe("Session", func() { Expect(sess.Context().Done()).To(BeClosed()) }) - It("closes streams with proper error", func() { + It("closes with an error", func() { testErr := errors.New("test error") - streamManager.EXPECT().CloseWithError(qerr.Error(0x1337, testErr.Error())) + streamManager.EXPECT().CloseWithError(qerr.ApplicationError(0x1337, testErr.Error())) expectReplaceWithClosed() cryptoSetup.EXPECT().Close() - packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil) + packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(f *wire.ConnectionCloseFrame) (*packedPacket, error) { + Expect(f.IsApplicationError).To(BeTrue()) + Expect(f.ErrorCode).To(BeEquivalentTo(0x1337)) + Expect(f.ReasonPhrase).To(Equal("test error")) + return &packedPacket{}, nil + }) sess.CloseWithError(0x1337, testErr.Error()) Eventually(areSessionsRunning).Should(BeFalse()) Expect(sess.Context().Done()).To(BeClosed()) @@ -1203,7 +1208,7 @@ var _ = Describe("Session", func() { defer GinkgoRecover() cryptoSetup.EXPECT().RunHandshake().MaxTimes(1) err := sess.run() - Expect(err).To(MatchError(qerr.Error(0x1337, testErr.Error()))) + Expect(err).To(MatchError(qerr.ApplicationError(0x1337, testErr.Error()))) close(done) }() streamManager.EXPECT().CloseWithError(gomock.Any()) From 6a9a591a10508c7d69595fa615dbbbf3e080f9eb Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 8 Nov 2019 13:48:50 +0700 Subject: [PATCH 2/2] use application-specific CONNECTION_CLOSE for normal termination --- session.go | 2 +- session_test.go | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/session.go b/session.go index 5353bcecf..a37b6b87a 100644 --- a/session.go +++ b/session.go @@ -1030,7 +1030,7 @@ func (s *session) CloseWithError(code protocol.ApplicationErrorCode, desc string func (s *session) handleCloseError(closeErr closeError) { if closeErr.err == nil { - closeErr.err = qerr.NoError + closeErr.err = qerr.ApplicationError(0, "") } var quicErr *qerr.QuicError diff --git a/session_test.go b/session_test.go index ea3e05f25..2ba573e89 100644 --- a/session_test.go +++ b/session_test.go @@ -426,10 +426,16 @@ var _ = Describe("Session", func() { }) It("shuts down without error", func() { - streamManager.EXPECT().CloseWithError(qerr.Error(qerr.NoError, "")) + streamManager.EXPECT().CloseWithError(qerr.ApplicationError(0, "")) expectReplaceWithClosed() cryptoSetup.EXPECT().Close() - packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{raw: []byte("connection close")}, nil) + packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(f *wire.ConnectionCloseFrame) (*packedPacket, error) { + Expect(f.IsApplicationError).To(BeTrue()) + Expect(f.ErrorCode).To(Equal(qerr.NoError)) + Expect(f.FrameType).To(BeZero()) + Expect(f.ReasonPhrase).To(BeEmpty()) + return &packedPacket{raw: []byte("connection close")}, nil + }) Expect(sess.Close()).To(Succeed()) Eventually(areSessionsRunning).Should(BeFalse()) Expect(mconn.written).To(HaveLen(1)) @@ -438,7 +444,7 @@ var _ = Describe("Session", func() { }) It("only closes once", func() { - streamManager.EXPECT().CloseWithError(qerr.Error(qerr.NoError, "")) + streamManager.EXPECT().CloseWithError(gomock.Any()) expectReplaceWithClosed() cryptoSetup.EXPECT().Close() packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil)