From 154b55b50ae8294b960ccee9523b3164a76f77c4 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 22 Mar 2021 14:22:36 +0800 Subject: [PATCH 1/2] introduce a qerr.Err{Idle,Handshake}Timeout --- internal/qerr/quic_error.go | 9 +++++++-- internal/qerr/quic_error_test.go | 2 +- session.go | 4 ++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/qerr/quic_error.go b/internal/qerr/quic_error.go index 18a26fdb..ae65a504 100644 --- a/internal/qerr/quic_error.go +++ b/internal/qerr/quic_error.go @@ -5,6 +5,11 @@ import ( "net" ) +var ( + ErrIdleTimeout = newTimeoutError("No recent network activity") + ErrHandshakeTimeout = newTimeoutError("Handshake did not complete in time") +) + // A QuicError consists of an error code plus a error reason type QuicError struct { ErrorCode ErrorCode @@ -33,8 +38,8 @@ func NewErrorWithFrameType(errorCode ErrorCode, frameType uint64, errorMessage s } } -// NewTimeoutError creates a new QuicError instance for a timeout error -func NewTimeoutError(errorMessage string) *QuicError { +// newTimeoutError creates a new QuicError instance for a timeout error +func newTimeoutError(errorMessage string) *QuicError { return &QuicError{ ErrorMessage: errorMessage, isTimeout: true, diff --git a/internal/qerr/quic_error_test.go b/internal/qerr/quic_error_test.go index 4d547287..1c1d93fc 100644 --- a/internal/qerr/quic_error_test.go +++ b/internal/qerr/quic_error_test.go @@ -31,7 +31,7 @@ var _ = Describe("QUIC Transport Errors", func() { }) It("has a string representation for timeout errors", func() { - err := NewTimeoutError("foobar") + err := newTimeoutError("foobar") Expect(err.Timeout()).To(BeTrue()) Expect(err.Error()).To(Equal("NO_ERROR: foobar")) }) diff --git a/session.go b/session.go index 4cc70238..708551d0 100644 --- a/session.go +++ b/session.go @@ -667,7 +667,7 @@ runLoop: if s.tracer != nil { s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonHandshake)) } - s.destroyImpl(qerr.NewTimeoutError("Handshake did not complete in time")) + s.destroyImpl(qerr.ErrHandshakeTimeout) continue } else { idleTimeoutStartTime := s.idleTimeoutStartTime() @@ -676,7 +676,7 @@ runLoop: if s.tracer != nil { s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonIdle)) } - s.destroyImpl(qerr.NewTimeoutError("No recent network activity")) + s.destroyImpl(qerr.ErrIdleTimeout) continue } } From 04074fbea3a51406efdb685aacb318c4c0a99548 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 22 Mar 2021 14:29:07 +0800 Subject: [PATCH 2/2] remove special treatment of timeout error when logging --- session.go | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/session.go b/session.go index 708551d0..32d7d7d3 100644 --- a/session.go +++ b/session.go @@ -664,18 +664,12 @@ runLoop: s.framer.QueueControlFrame(&wire.PingFrame{}) s.keepAlivePingSent = true } else if !s.handshakeComplete && now.Sub(s.sessionCreationTime) >= s.config.handshakeTimeout() { - if s.tracer != nil { - s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonHandshake)) - } s.destroyImpl(qerr.ErrHandshakeTimeout) continue } else { idleTimeoutStartTime := s.idleTimeoutStartTime() if (!s.handshakeComplete && now.Sub(idleTimeoutStartTime) >= s.config.HandshakeIdleTimeout) || (s.handshakeComplete && now.Sub(idleTimeoutStartTime) >= s.idleTimeout) { - if s.tracer != nil { - s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonIdle)) - } s.destroyImpl(qerr.ErrIdleTimeout) continue } @@ -1444,19 +1438,21 @@ func (s *session) handleCloseError(closeErr closeError) { } if s.tracer != nil { - // timeout errors are logged as soon as they occur (to distinguish between handshake and idle timeouts) - if nerr, ok := closeErr.err.(net.Error); !ok || !nerr.Timeout() { - var resetErr statelessResetErr - var vnErr errVersionNegotiation - if errors.As(closeErr.err, &resetErr) { - s.tracer.ClosedConnection(logging.NewStatelessResetCloseReason(resetErr.token)) - } else if errors.As(closeErr.err, &vnErr) { - s.tracer.ClosedConnection(logging.NewVersionNegotiationError(vnErr.theirVersions)) - } else if quicErr.IsApplicationError() { - s.tracer.ClosedConnection(logging.NewApplicationCloseReason(quicErr.ErrorCode, closeErr.remote)) - } else { - s.tracer.ClosedConnection(logging.NewTransportCloseReason(quicErr.ErrorCode, closeErr.remote)) - } + var resetErr statelessResetErr + var vnErr errVersionNegotiation + switch { + case errors.Is(closeErr.err, qerr.ErrIdleTimeout): + s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonIdle)) + case errors.Is(closeErr.err, qerr.ErrHandshakeTimeout): + s.tracer.ClosedConnection(logging.NewTimeoutCloseReason(logging.TimeoutReasonHandshake)) + case errors.As(closeErr.err, &resetErr): + s.tracer.ClosedConnection(logging.NewStatelessResetCloseReason(resetErr.token)) + case errors.As(closeErr.err, &vnErr): + s.tracer.ClosedConnection(logging.NewVersionNegotiationError(vnErr.theirVersions)) + case quicErr.IsApplicationError(): + s.tracer.ClosedConnection(logging.NewApplicationCloseReason(quicErr.ErrorCode, closeErr.remote)) + default: + s.tracer.ClosedConnection(logging.NewTransportCloseReason(quicErr.ErrorCode, closeErr.remote)) } }