From e9bced8d73d7cb9015024b0f130a7c2f9ebb22e5 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 11 Dec 2019 12:41:26 +0400 Subject: [PATCH 1/3] simplify populating the quic.Config --- client.go | 59 ++++--------------------------------------------------- server.go | 27 +++++++++++++------------ 2 files changed, 18 insertions(+), 68 deletions(-) diff --git a/client.go b/client.go index 019e574f8..533913bf9 100644 --- a/client.go +++ b/client.go @@ -198,62 +198,11 @@ func newClient( // populateClientConfig populates fields in the quic.Config with their default values, if none are set // it may be called with nil func populateClientConfig(config *Config, createdPacketConn bool) *Config { - if config == nil { - config = &Config{} - } - versions := config.Versions - if len(versions) == 0 { - versions = protocol.SupportedVersions - } - - handshakeTimeout := protocol.DefaultHandshakeTimeout - if config.HandshakeTimeout != 0 { - handshakeTimeout = config.HandshakeTimeout - } - idleTimeout := protocol.DefaultIdleTimeout - if config.IdleTimeout != 0 { - idleTimeout = config.IdleTimeout - } - - maxReceiveStreamFlowControlWindow := config.MaxReceiveStreamFlowControlWindow - if maxReceiveStreamFlowControlWindow == 0 { - maxReceiveStreamFlowControlWindow = protocol.DefaultMaxReceiveStreamFlowControlWindow - } - maxReceiveConnectionFlowControlWindow := config.MaxReceiveConnectionFlowControlWindow - if maxReceiveConnectionFlowControlWindow == 0 { - maxReceiveConnectionFlowControlWindow = protocol.DefaultMaxReceiveConnectionFlowControlWindow - } - maxIncomingStreams := config.MaxIncomingStreams - if maxIncomingStreams == 0 { - maxIncomingStreams = protocol.DefaultMaxIncomingStreams - } else if maxIncomingStreams < 0 { - maxIncomingStreams = 0 - } - maxIncomingUniStreams := config.MaxIncomingUniStreams - if maxIncomingUniStreams == 0 { - maxIncomingUniStreams = protocol.DefaultMaxIncomingUniStreams - } else if maxIncomingUniStreams < 0 { - maxIncomingUniStreams = 0 - } - connIDLen := config.ConnectionIDLength - if connIDLen == 0 && !createdPacketConn { - connIDLen = protocol.DefaultConnectionIDLength - } - - return &Config{ - Versions: versions, - HandshakeTimeout: handshakeTimeout, - IdleTimeout: idleTimeout, - ConnectionIDLength: connIDLen, - MaxReceiveStreamFlowControlWindow: maxReceiveStreamFlowControlWindow, - MaxReceiveConnectionFlowControlWindow: maxReceiveConnectionFlowControlWindow, - MaxIncomingStreams: maxIncomingStreams, - MaxIncomingUniStreams: maxIncomingUniStreams, - KeepAlive: config.KeepAlive, - StatelessResetKey: config.StatelessResetKey, - QuicTracer: config.QuicTracer, - TokenStore: config.TokenStore, + config = populateConfig(config) + if config.ConnectionIDLength == 0 && !createdPacketConn { + config.ConnectionIDLength = protocol.DefaultConnectionIDLength } + return config } func (c *client) dial(ctx context.Context) error { diff --git a/server.go b/server.go index 9b6c1abc9..0518ae19a 100644 --- a/server.go +++ b/server.go @@ -208,6 +208,17 @@ var defaultAcceptToken = func(clientAddr net.Addr, token *Token) bool { // populateServerConfig populates fields in the quic.Config with their default values, if none are set // it may be called with nil func populateServerConfig(config *Config) *Config { + config = populateConfig(config) + if config.ConnectionIDLength == 0 { + config.ConnectionIDLength = protocol.DefaultConnectionIDLength + } + if config.AcceptToken == nil { + config.AcceptToken = defaultAcceptToken + } + return config +} + +func populateConfig(config *Config) *Config { if config == nil { config = &Config{} } @@ -215,12 +226,6 @@ func populateServerConfig(config *Config) *Config { if len(versions) == 0 { versions = protocol.SupportedVersions } - - verifyToken := defaultAcceptToken - if config.AcceptToken != nil { - verifyToken = config.AcceptToken - } - handshakeTimeout := protocol.DefaultHandshakeTimeout if config.HandshakeTimeout != 0 { handshakeTimeout = config.HandshakeTimeout @@ -229,7 +234,6 @@ func populateServerConfig(config *Config) *Config { if config.IdleTimeout != 0 { idleTimeout = config.IdleTimeout } - maxReceiveStreamFlowControlWindow := config.MaxReceiveStreamFlowControlWindow if maxReceiveStreamFlowControlWindow == 0 { maxReceiveStreamFlowControlWindow = protocol.DefaultMaxReceiveStreamFlowControlWindow @@ -250,23 +254,20 @@ func populateServerConfig(config *Config) *Config { } else if maxIncomingUniStreams < 0 { maxIncomingUniStreams = 0 } - connIDLen := config.ConnectionIDLength - if connIDLen == 0 { - connIDLen = protocol.DefaultConnectionIDLength - } return &Config{ Versions: versions, HandshakeTimeout: handshakeTimeout, IdleTimeout: idleTimeout, - AcceptToken: verifyToken, + AcceptToken: config.AcceptToken, KeepAlive: config.KeepAlive, MaxReceiveStreamFlowControlWindow: maxReceiveStreamFlowControlWindow, MaxReceiveConnectionFlowControlWindow: maxReceiveConnectionFlowControlWindow, MaxIncomingStreams: maxIncomingStreams, MaxIncomingUniStreams: maxIncomingUniStreams, - ConnectionIDLength: connIDLen, + ConnectionIDLength: config.ConnectionIDLength, StatelessResetKey: config.StatelessResetKey, + TokenStore: config.TokenStore, QuicTracer: config.QuicTracer, } } From 8dcca046e3e02f6bc625523275e254ea701d85bc Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 11 Dec 2019 13:50:13 +0400 Subject: [PATCH 2/3] don't set the idle timeout timer before the handshake completes --- session.go | 15 ++++++++------- session_test.go | 11 ++++++----- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/session.go b/session.go index 68d7407bb..ff1843e6f 100644 --- a/session.go +++ b/session.go @@ -569,10 +569,15 @@ func (s *session) ConnectionState() tls.ConnectionState { func (s *session) maybeResetTimer() { var deadline time.Time - if s.config.KeepAlive && s.handshakeComplete && !s.keepAlivePingSent { - deadline = s.idleTimeoutStartTime().Add(s.keepAliveInterval / 2) + if !s.handshakeComplete { + handshakeDeadline := s.sessionCreationTime.Add(s.config.HandshakeTimeout) + deadline = handshakeDeadline } else { - deadline = s.idleTimeoutStartTime().Add(s.config.IdleTimeout) + if s.config.KeepAlive && !s.keepAlivePingSent { + deadline = s.idleTimeoutStartTime().Add(s.keepAliveInterval / 2) + } else { + deadline = s.idleTimeoutStartTime().Add(s.config.IdleTimeout) + } } if ackAlarm := s.receivedPacketHandler.GetAlarmTimeout(); !ackAlarm.IsZero() { @@ -581,10 +586,6 @@ func (s *session) maybeResetTimer() { if lossTime := s.sentPacketHandler.GetLossDetectionTimeout(); !lossTime.IsZero() { deadline = utils.MinTime(deadline, lossTime) } - if !s.handshakeComplete { - handshakeDeadline := s.sessionCreationTime.Add(s.config.HandshakeTimeout) - deadline = utils.MinTime(deadline, handshakeDeadline) - } if !s.pacingDeadline.IsZero() { deadline = utils.MinTime(deadline, s.pacingDeadline) } diff --git a/session_test.go b/session_test.go index b277b7326..4602dbb07 100644 --- a/session_test.go +++ b/session_test.go @@ -131,6 +131,7 @@ var _ = Describe("Session", func() { sess.packer = packer cryptoSetup = mocks.NewMockCryptoSetup(mockCtrl) sess.cryptoStreamHandler = cryptoSetup + sess.handshakeComplete = true }) AfterEach(func() { @@ -466,7 +467,6 @@ var _ = Describe("Session", func() { }) It("closes with an error", func() { - sess.handshakeComplete = true streamManager.EXPECT().CloseWithError(qerr.ApplicationError(0x1337, "test error")) expectReplaceWithClosed() cryptoSetup.EXPECT().Close() @@ -482,7 +482,6 @@ var _ = Describe("Session", func() { }) It("includes the frame type in transport-level close frames", func() { - sess.handshakeComplete = true testErr := qerr.ErrorWithFrameType(0x1337, 0x42, "test error") streamManager.EXPECT().CloseWithError(testErr) expectReplaceWithClosed() @@ -500,6 +499,7 @@ var _ = Describe("Session", func() { }) It("doesn't send application-level error before the handshake completes", func() { + sess.handshakeComplete = false streamManager.EXPECT().CloseWithError(qerr.ApplicationError(0x1337, "test error")) expectReplaceWithClosed() cryptoSetup.EXPECT().Close() @@ -764,6 +764,7 @@ var _ = Describe("Session", func() { }) It("queues undecryptable packets", func() { + sess.handshakeComplete = false hdr := &wire.ExtendedHeader{ Header: wire.Header{ IsLongHeader: true, @@ -856,6 +857,7 @@ var _ = Describe("Session", func() { }) It("works with undecryptable packets", func() { + sess.handshakeComplete = false hdrLen1, packet1 := getPacketWithLength(srcConnID, 456) hdrLen2, packet2 := getPacketWithLength(srcConnID, 123) gomock.InOrder( @@ -1371,7 +1373,6 @@ var _ = Describe("Session", func() { BeforeEach(func() { sess.config.KeepAlive = true - sess.handshakeComplete = true }) AfterEach(func() { @@ -1433,7 +1434,6 @@ var _ = Describe("Session", func() { It("times out due to no network activity", func() { sessionRunner.EXPECT().Remove(gomock.Any()).Times(2) - sess.handshakeComplete = true sess.lastPacketReceivedTime = time.Now().Add(-time.Hour) done := make(chan struct{}) cryptoSetup.EXPECT().Close() @@ -1451,6 +1451,7 @@ var _ = Describe("Session", func() { }) It("times out due to non-completed handshake", func() { + sess.handshakeComplete = false sess.sessionCreationTime = time.Now().Add(-protocol.DefaultHandshakeTimeout).Add(-time.Second) sessionRunner.EXPECT().Remove(gomock.Any()).Times(2) cryptoSetup.EXPECT().Close() @@ -1469,6 +1470,7 @@ var _ = Describe("Session", func() { }) It("does not use the idle timeout before the handshake complete", func() { + sess.handshakeComplete = false sess.config.IdleTimeout = 9999 * time.Second sess.lastPacketReceivedTime = time.Now().Add(-time.Minute) packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(f *wire.ConnectionCloseFrame) (*packedPacket, error) { @@ -1515,7 +1517,6 @@ var _ = Describe("Session", func() { }) It("doesn't time out when it just sent a packet", func() { - sess.handshakeComplete = true sess.lastPacketReceivedTime = time.Now().Add(-time.Hour) sess.firstAckElicitingPacketAfterIdleSentTime = time.Now().Add(-time.Second) sess.config.IdleTimeout = 30 * time.Second From 27549c56656665859354255d3912f6428bfcb9f0 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 11 Dec 2019 14:03:30 +0400 Subject: [PATCH 3/3] use the minimum of the two peers' max_idle_timeouts --- client_test.go | 6 ++--- http3/client_test.go | 8 +++--- integrationtests/self/http_test.go | 4 +-- integrationtests/self/mitm_test.go | 2 +- integrationtests/self/stateless_reset_test.go | 2 +- integrationtests/self/timeout_test.go | 6 ++--- interface.go | 5 ++-- internal/handshake/crypto_setup_test.go | 8 +++--- .../handshake/transport_parameter_test.go | 12 ++++----- internal/handshake/transport_parameters.go | 16 ++++++------ internal/utils/minmax.go | 11 ++++++++ internal/utils/minmax_test.go | 11 +++++++- server.go | 6 ++--- server_test.go | 6 ++--- session.go | 13 ++++++---- session_test.go | 25 ++++++++++++++----- 16 files changed, 89 insertions(+), 52 deletions(-) diff --git a/client_test.go b/client_test.go index bf7961c45..a720d7a63 100644 --- a/client_test.go +++ b/client_test.go @@ -398,7 +398,7 @@ var _ = Describe("Client", func() { tokenStore := NewLRUTokenStore(10, 4) config := &Config{ HandshakeTimeout: 1337 * time.Minute, - IdleTimeout: 42 * time.Hour, + MaxIdleTimeout: 42 * time.Hour, MaxIncomingStreams: 1234, MaxIncomingUniStreams: 4321, ConnectionIDLength: 13, @@ -408,7 +408,7 @@ var _ = Describe("Client", func() { } c := populateClientConfig(config, false) Expect(c.HandshakeTimeout).To(Equal(1337 * time.Minute)) - Expect(c.IdleTimeout).To(Equal(42 * time.Hour)) + Expect(c.MaxIdleTimeout).To(Equal(42 * time.Hour)) Expect(c.MaxIncomingStreams).To(Equal(1234)) Expect(c.MaxIncomingUniStreams).To(Equal(4321)) Expect(c.ConnectionIDLength).To(Equal(13)) @@ -456,7 +456,7 @@ var _ = Describe("Client", func() { c := populateClientConfig(&Config{}, false) Expect(c.Versions).To(Equal(protocol.SupportedVersions)) Expect(c.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout)) - Expect(c.IdleTimeout).To(Equal(protocol.DefaultIdleTimeout)) + Expect(c.MaxIdleTimeout).To(Equal(protocol.DefaultIdleTimeout)) }) }) diff --git a/http3/client_test.go b/http3/client_test.go index 607aba057..1ce69aee8 100644 --- a/http3/client_test.go +++ b/http3/client_test.go @@ -75,7 +75,7 @@ var _ = Describe("Client", func() { ServerName: "foo.bar", NextProtos: []string{"proto foo", "proto bar"}, } - quicConf := &quic.Config{IdleTimeout: time.Nanosecond} + quicConf := &quic.Config{MaxIdleTimeout: time.Nanosecond} client = newClient("localhost:1337", tlsConf, &roundTripperOpts{}, quicConf, nil) var dialAddrCalled bool dialAddr = func( @@ -86,7 +86,7 @@ var _ = Describe("Client", func() { Expect(hostname).To(Equal("localhost:1337")) Expect(tlsConfP.ServerName).To(Equal(tlsConf.ServerName)) Expect(tlsConfP.NextProtos).To(Equal([]string{nextProtoH3})) - Expect(quicConfP.IdleTimeout).To(Equal(quicConf.IdleTimeout)) + Expect(quicConfP.MaxIdleTimeout).To(Equal(quicConf.MaxIdleTimeout)) dialAddrCalled = true return nil, errors.New("test done") } @@ -99,13 +99,13 @@ var _ = Describe("Client", func() { It("uses the custom dialer, if provided", func() { testErr := errors.New("test done") tlsConf := &tls.Config{ServerName: "foo.bar"} - quicConf := &quic.Config{IdleTimeout: 1337 * time.Second} + quicConf := &quic.Config{MaxIdleTimeout: 1337 * time.Second} var dialerCalled bool dialer := func(network, address string, tlsConfP *tls.Config, quicConfP *quic.Config) (quic.Session, error) { Expect(network).To(Equal("udp")) Expect(address).To(Equal("localhost:1337")) Expect(tlsConfP.ServerName).To(Equal("foo.bar")) - Expect(quicConfP.IdleTimeout).To(Equal(quicConf.IdleTimeout)) + Expect(quicConfP.MaxIdleTimeout).To(Equal(quicConf.MaxIdleTimeout)) dialerCalled = true return nil, testErr } diff --git a/integrationtests/self/http_test.go b/integrationtests/self/http_test.go index 97c2bf41c..6b2337c1e 100644 --- a/integrationtests/self/http_test.go +++ b/integrationtests/self/http_test.go @@ -111,8 +111,8 @@ var _ = Describe("HTTP tests", func() { }, DisableCompression: true, QuicConfig: &quic.Config{ - Versions: []protocol.VersionNumber{version}, - IdleTimeout: 10 * time.Second, + Versions: []protocol.VersionNumber{version}, + MaxIdleTimeout: 10 * time.Second, }, }, } diff --git a/integrationtests/self/mitm_test.go b/integrationtests/self/mitm_test.go index 38d156fbe..0c5940276 100644 --- a/integrationtests/self/mitm_test.go +++ b/integrationtests/self/mitm_test.go @@ -222,7 +222,7 @@ var _ = Describe("MITM test", func() { BeforeEach(func() { numCorrupted = 0 - serverConfig.IdleTimeout = idleTimeout + serverConfig.MaxIdleTimeout = idleTimeout }) AfterEach(func() { diff --git a/integrationtests/self/stateless_reset_test.go b/integrationtests/self/stateless_reset_test.go index 3fbd404ae..34cf52364 100644 --- a/integrationtests/self/stateless_reset_test.go +++ b/integrationtests/self/stateless_reset_test.go @@ -60,7 +60,7 @@ var _ = Describe("Stateless Resets", func() { getTLSClientConfig(), &quic.Config{ ConnectionIDLength: connIDLen, - IdleTimeout: 2 * time.Second, + MaxIdleTimeout: 2 * time.Second, }, ) Expect(err).ToNot(HaveOccurred()) diff --git a/integrationtests/self/timeout_test.go b/integrationtests/self/timeout_test.go index b4d7cf78f..fd05721f3 100644 --- a/integrationtests/self/timeout_test.go +++ b/integrationtests/self/timeout_test.go @@ -92,7 +92,7 @@ var _ = Describe("Timeout tests", func() { sess, err := quic.DialAddr( fmt.Sprintf("localhost:%d", proxy.LocalPort()), getTLSClientConfig(), - &quic.Config{IdleTimeout: idleTimeout}, + &quic.Config{MaxIdleTimeout: idleTimeout}, ) Expect(err).ToNot(HaveOccurred()) strIn, err := sess.AcceptStream(context.Background()) @@ -155,7 +155,7 @@ var _ = Describe("Timeout tests", func() { sess, err := quic.DialAddr( fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port), getTLSClientConfig(), - &quic.Config{IdleTimeout: idleTimeout}, + &quic.Config{MaxIdleTimeout: idleTimeout}, ) Expect(err).ToNot(HaveOccurred()) startTime := time.Now() @@ -196,7 +196,7 @@ var _ = Describe("Timeout tests", func() { sess, err := quic.DialAddr( fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port), getTLSClientConfig(), - &quic.Config{IdleTimeout: idleTimeout}, + &quic.Config{MaxIdleTimeout: idleTimeout}, ) Expect(err).ToNot(HaveOccurred()) diff --git a/interface.go b/interface.go index f7fe44d42..7f201e04c 100644 --- a/interface.go +++ b/interface.go @@ -218,11 +218,12 @@ type Config struct { // If the timeout is exceeded, the connection is closed. // If this value is zero, the timeout is set to 10 seconds. HandshakeTimeout time.Duration - // IdleTimeout is the maximum duration that may pass without any incoming network activity. + // MaxIdleTimeout is the maximum duration that may pass without any incoming network activity. + // The actual value for the idle timeout is the minimum of this value and the peer's. // This value only applies after the handshake has completed. // If the timeout is exceeded, the connection is closed. // If this value is zero, the timeout is set to 30 seconds. - IdleTimeout time.Duration + MaxIdleTimeout time.Duration // AcceptToken determines if a Token is accepted. // It is called with token = nil if the client didn't send a token. // If not set, a default verification function is used: diff --git a/internal/handshake/crypto_setup_test.go b/internal/handshake/crypto_setup_test.go index 9f6aa92ef..a870ac2b1 100644 --- a/internal/handshake/crypto_setup_test.go +++ b/internal/handshake/crypto_setup_test.go @@ -424,7 +424,7 @@ var _ = Describe("Crypto Setup TLS", func() { It("receives transport parameters", func() { var cTransportParametersRcvd, sTransportParametersRcvd []byte cChunkChan, cInitialStream, cHandshakeStream, _ := initStreams() - cTransportParameters := &TransportParameters{IdleTimeout: 0x42 * time.Second} + cTransportParameters := &TransportParameters{MaxIdleTimeout: 0x42 * time.Second} cRunner := NewMockHandshakeRunner(mockCtrl) cRunner.EXPECT().OnReceivedParams(gomock.Any()).Do(func(b []byte) { sTransportParametersRcvd = b }) cRunner.EXPECT().OnHandshakeComplete() @@ -447,7 +447,7 @@ var _ = Describe("Crypto Setup TLS", func() { sRunner.EXPECT().OnReceivedParams(gomock.Any()).Do(func(b []byte) { cTransportParametersRcvd = b }) sRunner.EXPECT().OnHandshakeComplete() sTransportParameters := &TransportParameters{ - IdleTimeout: 0x1337 * time.Second, + MaxIdleTimeout: 0x1337 * time.Second, StatelessResetToken: &token, } server := NewCryptoSetupServer( @@ -473,11 +473,11 @@ var _ = Describe("Crypto Setup TLS", func() { Expect(cTransportParametersRcvd).ToNot(BeNil()) clTP := &TransportParameters{} Expect(clTP.Unmarshal(cTransportParametersRcvd, protocol.PerspectiveClient)).To(Succeed()) - Expect(clTP.IdleTimeout).To(Equal(cTransportParameters.IdleTimeout)) + Expect(clTP.MaxIdleTimeout).To(Equal(cTransportParameters.MaxIdleTimeout)) Expect(sTransportParametersRcvd).ToNot(BeNil()) srvTP := &TransportParameters{} Expect(srvTP.Unmarshal(sTransportParametersRcvd, protocol.PerspectiveServer)).To(Succeed()) - Expect(srvTP.IdleTimeout).To(Equal(sTransportParameters.IdleTimeout)) + Expect(srvTP.MaxIdleTimeout).To(Equal(sTransportParameters.MaxIdleTimeout)) }) Context("with session tickets", func() { diff --git a/internal/handshake/transport_parameter_test.go b/internal/handshake/transport_parameter_test.go index 88d767b9a..5168b1502 100644 --- a/internal/handshake/transport_parameter_test.go +++ b/internal/handshake/transport_parameter_test.go @@ -29,14 +29,14 @@ var _ = Describe("Transport Parameters", func() { InitialMaxData: 0x4567, MaxBidiStreamNum: 1337, MaxUniStreamNum: 7331, - IdleTimeout: 42 * time.Second, + MaxIdleTimeout: 42 * time.Second, OriginalConnectionID: protocol.ConnectionID{0xde, 0xad, 0xbe, 0xef}, AckDelayExponent: 14, MaxAckDelay: 37 * time.Millisecond, StatelessResetToken: &[16]byte{0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x00}, ActiveConnectionIDLimit: 123, } - Expect(p.String()).To(Equal("&handshake.TransportParameters{OriginalConnectionID: 0xdeadbeef, InitialMaxStreamDataBidiLocal: 0x1234, InitialMaxStreamDataBidiRemote: 0x2345, InitialMaxStreamDataUni: 0x3456, InitialMaxData: 0x4567, MaxBidiStreamNum: 1337, MaxUniStreamNum: 7331, IdleTimeout: 42s, AckDelayExponent: 14, MaxAckDelay: 37ms, ActiveConnectionIDLimit: 123, StatelessResetToken: 0x112233445566778899aabbccddeeff00}")) + Expect(p.String()).To(Equal("&handshake.TransportParameters{OriginalConnectionID: 0xdeadbeef, InitialMaxStreamDataBidiLocal: 0x1234, InitialMaxStreamDataBidiRemote: 0x2345, InitialMaxStreamDataUni: 0x3456, InitialMaxData: 0x4567, MaxBidiStreamNum: 1337, MaxUniStreamNum: 7331, MaxIdleTimeout: 42s, AckDelayExponent: 14, MaxAckDelay: 37ms, ActiveConnectionIDLimit: 123, StatelessResetToken: 0x112233445566778899aabbccddeeff00}")) }) It("has a string representation, if there's no stateless reset token", func() { @@ -47,13 +47,13 @@ var _ = Describe("Transport Parameters", func() { InitialMaxData: 0x4567, MaxBidiStreamNum: 1337, MaxUniStreamNum: 7331, - IdleTimeout: 42 * time.Second, + MaxIdleTimeout: 42 * time.Second, OriginalConnectionID: protocol.ConnectionID{0xde, 0xad, 0xbe, 0xef}, AckDelayExponent: 14, MaxAckDelay: 37 * time.Second, ActiveConnectionIDLimit: 89, } - Expect(p.String()).To(Equal("&handshake.TransportParameters{OriginalConnectionID: 0xdeadbeef, InitialMaxStreamDataBidiLocal: 0x1234, InitialMaxStreamDataBidiRemote: 0x2345, InitialMaxStreamDataUni: 0x3456, InitialMaxData: 0x4567, MaxBidiStreamNum: 1337, MaxUniStreamNum: 7331, IdleTimeout: 42s, AckDelayExponent: 14, MaxAckDelay: 37s, ActiveConnectionIDLimit: 89}")) + Expect(p.String()).To(Equal("&handshake.TransportParameters{OriginalConnectionID: 0xdeadbeef, InitialMaxStreamDataBidiLocal: 0x1234, InitialMaxStreamDataBidiRemote: 0x2345, InitialMaxStreamDataUni: 0x3456, InitialMaxData: 0x4567, MaxBidiStreamNum: 1337, MaxUniStreamNum: 7331, MaxIdleTimeout: 42s, AckDelayExponent: 14, MaxAckDelay: 37s, ActiveConnectionIDLimit: 89}")) }) It("marshals and unmarshals", func() { @@ -70,7 +70,7 @@ var _ = Describe("Transport Parameters", func() { InitialMaxStreamDataBidiRemote: protocol.ByteCount(getRandomValue()), InitialMaxStreamDataUni: protocol.ByteCount(getRandomValue()), InitialMaxData: protocol.ByteCount(getRandomValue()), - IdleTimeout: 0xcafe * time.Second, + MaxIdleTimeout: 0xcafe * time.Second, MaxBidiStreamNum: protocol.StreamNum(getRandomValue()), MaxUniStreamNum: protocol.StreamNum(getRandomValue()), DisableMigration: true, @@ -90,7 +90,7 @@ var _ = Describe("Transport Parameters", func() { Expect(p.InitialMaxData).To(Equal(params.InitialMaxData)) Expect(p.MaxUniStreamNum).To(Equal(params.MaxUniStreamNum)) Expect(p.MaxBidiStreamNum).To(Equal(params.MaxBidiStreamNum)) - Expect(p.IdleTimeout).To(Equal(params.IdleTimeout)) + Expect(p.MaxIdleTimeout).To(Equal(params.MaxIdleTimeout)) Expect(p.DisableMigration).To(Equal(params.DisableMigration)) Expect(p.StatelessResetToken).To(Equal(params.StatelessResetToken)) Expect(p.OriginalConnectionID).To(Equal(protocol.ConnectionID{0xde, 0xad, 0xbe, 0xef})) diff --git a/internal/handshake/transport_parameters.go b/internal/handshake/transport_parameters.go index 502dedb30..0d1337ca9 100644 --- a/internal/handshake/transport_parameters.go +++ b/internal/handshake/transport_parameters.go @@ -25,7 +25,7 @@ type transportParameterID uint16 const ( originalConnectionIDParameterID transportParameterID = 0x0 - idleTimeoutParameterID transportParameterID = 0x1 + maxIdleTimeoutParameterID transportParameterID = 0x1 statelessResetTokenParameterID transportParameterID = 0x2 maxPacketSizeParameterID transportParameterID = 0x3 initialMaxDataParameterID transportParameterID = 0x4 @@ -68,7 +68,7 @@ type TransportParameters struct { MaxUniStreamNum protocol.StreamNum MaxBidiStreamNum protocol.StreamNum - IdleTimeout time.Duration + MaxIdleTimeout time.Duration PreferredAddress *PreferredAddress @@ -123,7 +123,7 @@ func (p *TransportParameters) unmarshal(data []byte, sentBy protocol.Perspective initialMaxDataParameterID, initialMaxStreamsBidiParameterID, initialMaxStreamsUniParameterID, - idleTimeoutParameterID, + maxIdleTimeoutParameterID, maxPacketSizeParameterID, activeConnectionIDLimitParameterID: if err := p.readNumericTransportParameter(r, paramID, int(paramLen)); err != nil { @@ -259,8 +259,8 @@ func (p *TransportParameters) readNumericTransportParameter( p.MaxBidiStreamNum = protocol.StreamNum(val) case initialMaxStreamsUniParameterID: p.MaxUniStreamNum = protocol.StreamNum(val) - case idleTimeoutParameterID: - p.IdleTimeout = utils.MaxDuration(protocol.MinRemoteIdleTimeout, time.Duration(val)*time.Millisecond) + case maxIdleTimeoutParameterID: + p.MaxIdleTimeout = utils.MaxDuration(protocol.MinRemoteIdleTimeout, time.Duration(val)*time.Millisecond) case maxPacketSizeParameterID: if val < 1200 { return fmt.Errorf("invalid value for max_packet_size: %d (minimum 1200)", val) @@ -314,7 +314,7 @@ func (p *TransportParameters) Marshal() []byte { // initial_max_uni_streams p.marshalVarintParam(b, initialMaxStreamsUniParameterID, uint64(p.MaxUniStreamNum)) // idle_timeout - p.marshalVarintParam(b, idleTimeoutParameterID, uint64(p.IdleTimeout/time.Millisecond)) + p.marshalVarintParam(b, maxIdleTimeoutParameterID, uint64(p.MaxIdleTimeout/time.Millisecond)) // max_packet_size p.marshalVarintParam(b, maxPacketSizeParameterID, uint64(protocol.MaxReceivePacketSize)) // max_ack_delay @@ -371,8 +371,8 @@ func (p *TransportParameters) marshalVarintParam(b *bytes.Buffer, id transportPa // String returns a string representation, intended for logging. func (p *TransportParameters) String() string { - logString := "&handshake.TransportParameters{OriginalConnectionID: %s, InitialMaxStreamDataBidiLocal: %#x, InitialMaxStreamDataBidiRemote: %#x, InitialMaxStreamDataUni: %#x, InitialMaxData: %#x, MaxBidiStreamNum: %d, MaxUniStreamNum: %d, IdleTimeout: %s, AckDelayExponent: %d, MaxAckDelay: %s, ActiveConnectionIDLimit: %d" - logParams := []interface{}{p.OriginalConnectionID, p.InitialMaxStreamDataBidiLocal, p.InitialMaxStreamDataBidiRemote, p.InitialMaxStreamDataUni, p.InitialMaxData, p.MaxBidiStreamNum, p.MaxUniStreamNum, p.IdleTimeout, p.AckDelayExponent, p.MaxAckDelay, p.ActiveConnectionIDLimit} + logString := "&handshake.TransportParameters{OriginalConnectionID: %s, InitialMaxStreamDataBidiLocal: %#x, InitialMaxStreamDataBidiRemote: %#x, InitialMaxStreamDataUni: %#x, InitialMaxData: %#x, MaxBidiStreamNum: %d, MaxUniStreamNum: %d, MaxIdleTimeout: %s, AckDelayExponent: %d, MaxAckDelay: %s, ActiveConnectionIDLimit: %d" + logParams := []interface{}{p.OriginalConnectionID, p.InitialMaxStreamDataBidiLocal, p.InitialMaxStreamDataBidiRemote, p.InitialMaxStreamDataUni, p.InitialMaxData, p.MaxBidiStreamNum, p.MaxUniStreamNum, p.MaxIdleTimeout, p.AckDelayExponent, p.MaxAckDelay, p.ActiveConnectionIDLimit} if p.StatelessResetToken != nil { // the client never sends a stateless reset token logString += ", StatelessResetToken: %#x" logParams = append(logParams, *p.StatelessResetToken) diff --git a/internal/utils/minmax.go b/internal/utils/minmax.go index 84cbec7b9..ee1f85f62 100644 --- a/internal/utils/minmax.go +++ b/internal/utils/minmax.go @@ -106,6 +106,17 @@ func MinDuration(a, b time.Duration) time.Duration { return a } +// MinNonZeroDuration return the minimum duration that's not zero. +func MinNonZeroDuration(a, b time.Duration) time.Duration { + if a == 0 { + return b + } + if b == 0 { + return a + } + return MinDuration(a, b) +} + // AbsDuration returns the absolute value of a time duration func AbsDuration(d time.Duration) time.Duration { if d >= 0 { diff --git a/internal/utils/minmax_test.go b/internal/utils/minmax_test.go index 16b1f57a3..883d0349b 100644 --- a/internal/utils/minmax_test.go +++ b/internal/utils/minmax_test.go @@ -89,13 +89,22 @@ var _ = Describe("Min / Max", func() { Expect(MinPacketNumber(2, 1)).To(Equal(protocol.PacketNumber(1))) }) - It("returns the minimum time", func() { + It("returns the minimum duration", func() { a := time.Now() b := a.Add(time.Second) Expect(MinTime(a, b)).To(Equal(a)) Expect(MinTime(b, a)).To(Equal(a)) }) + It("returns the minium non-zero duration", func() { + var a time.Duration + b := time.Second + Expect(MinNonZeroDuration(0, 0)).To(BeZero()) + Expect(MinNonZeroDuration(a, b)).To(Equal(b)) + Expect(MinNonZeroDuration(b, a)).To(Equal(b)) + Expect(MinNonZeroDuration(time.Minute, time.Hour)).To(Equal(time.Minute)) + }) + It("returns the minium non-zero time", func() { a := time.Time{} b := time.Now() diff --git a/server.go b/server.go index 0518ae19a..9dea3694c 100644 --- a/server.go +++ b/server.go @@ -231,8 +231,8 @@ func populateConfig(config *Config) *Config { handshakeTimeout = config.HandshakeTimeout } idleTimeout := protocol.DefaultIdleTimeout - if config.IdleTimeout != 0 { - idleTimeout = config.IdleTimeout + if config.MaxIdleTimeout != 0 { + idleTimeout = config.MaxIdleTimeout } maxReceiveStreamFlowControlWindow := config.MaxReceiveStreamFlowControlWindow if maxReceiveStreamFlowControlWindow == 0 { @@ -258,7 +258,7 @@ func populateConfig(config *Config) *Config { return &Config{ Versions: versions, HandshakeTimeout: handshakeTimeout, - IdleTimeout: idleTimeout, + MaxIdleTimeout: idleTimeout, AcceptToken: config.AcceptToken, KeepAlive: config.KeepAlive, MaxReceiveStreamFlowControlWindow: maxReceiveStreamFlowControlWindow, diff --git a/server_test.go b/server_test.go index 2b0d0d472..5858fe9b8 100644 --- a/server_test.go +++ b/server_test.go @@ -95,7 +95,7 @@ var _ = Describe("Server", func() { server := ln.(*baseServer) Expect(server.config.Versions).To(Equal(protocol.SupportedVersions)) Expect(server.config.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout)) - Expect(server.config.IdleTimeout).To(Equal(protocol.DefaultIdleTimeout)) + Expect(server.config.MaxIdleTimeout).To(Equal(protocol.DefaultIdleTimeout)) Expect(reflect.ValueOf(server.config.AcceptToken)).To(Equal(reflect.ValueOf(defaultAcceptToken))) Expect(server.config.KeepAlive).To(BeFalse()) // stop the listener @@ -110,7 +110,7 @@ var _ = Describe("Server", func() { Versions: supportedVersions, AcceptToken: acceptToken, HandshakeTimeout: 1337 * time.Hour, - IdleTimeout: 42 * time.Minute, + MaxIdleTimeout: 42 * time.Minute, KeepAlive: true, StatelessResetKey: []byte("foobar"), QuicTracer: tracer, @@ -121,7 +121,7 @@ var _ = Describe("Server", func() { Expect(server.sessionHandler).ToNot(BeNil()) Expect(server.config.Versions).To(Equal(supportedVersions)) Expect(server.config.HandshakeTimeout).To(Equal(1337 * time.Hour)) - Expect(server.config.IdleTimeout).To(Equal(42 * time.Minute)) + Expect(server.config.MaxIdleTimeout).To(Equal(42 * time.Minute)) Expect(reflect.ValueOf(server.config.AcceptToken)).To(Equal(reflect.ValueOf(acceptToken))) Expect(server.config.KeepAlive).To(BeTrue()) Expect(server.config.StatelessResetKey).To(Equal([]byte("foobar"))) diff --git a/session.go b/session.go index ff1843e6f..aaf770c21 100644 --- a/session.go +++ b/session.go @@ -163,6 +163,7 @@ type session struct { receivedRetry bool receivedFirstPacket bool + idleTimeout time.Duration sessionCreationTime time.Time // The idle timeout is set based on the max of the time we received the last packet... lastPacketReceivedTime time.Time @@ -245,7 +246,7 @@ var newSession = func( InitialMaxStreamDataBidiRemote: protocol.InitialMaxStreamData, InitialMaxStreamDataUni: protocol.InitialMaxStreamData, InitialMaxData: protocol.InitialMaxData, - IdleTimeout: s.config.IdleTimeout, + MaxIdleTimeout: s.config.MaxIdleTimeout, MaxBidiStreamNum: protocol.StreamNum(s.config.MaxIncomingStreams), MaxUniStreamNum: protocol.StreamNum(s.config.MaxIncomingUniStreams), MaxAckDelay: protocol.MaxAckDelayInclGranularity, @@ -346,7 +347,7 @@ var newClientSession = func( InitialMaxStreamDataBidiLocal: protocol.InitialMaxStreamData, InitialMaxStreamDataUni: protocol.InitialMaxStreamData, InitialMaxData: protocol.InitialMaxData, - IdleTimeout: s.config.IdleTimeout, + MaxIdleTimeout: s.config.MaxIdleTimeout, MaxBidiStreamNum: protocol.StreamNum(s.config.MaxIncomingStreams), MaxUniStreamNum: protocol.StreamNum(s.config.MaxIncomingUniStreams), MaxAckDelay: protocol.MaxAckDelayInclGranularity, @@ -533,7 +534,7 @@ runLoop: s.destroyImpl(qerr.TimeoutError("Handshake did not complete in time")) continue } - if s.handshakeComplete && now.Sub(s.idleTimeoutStartTime()) >= s.config.IdleTimeout { + if s.handshakeComplete && now.Sub(s.idleTimeoutStartTime()) >= s.idleTimeout { s.destroyImpl(qerr.TimeoutError("No recent network activity")) continue } @@ -576,7 +577,7 @@ func (s *session) maybeResetTimer() { if s.config.KeepAlive && !s.keepAlivePingSent { deadline = s.idleTimeoutStartTime().Add(s.keepAliveInterval / 2) } else { - deadline = s.idleTimeoutStartTime().Add(s.config.IdleTimeout) + deadline = s.idleTimeoutStartTime().Add(s.idleTimeout) } } @@ -1094,7 +1095,9 @@ func (s *session) processTransportParameters(data []byte) { } s.logger.Debugf("Received Transport Parameters: %s", params) s.peerParams = params - s.keepAliveInterval = utils.MinDuration(params.IdleTimeout/2, protocol.MaxKeepAliveInterval) + // Our local idle timeout will always be > 0. + s.idleTimeout = utils.MinNonZeroDuration(s.config.MaxIdleTimeout, params.MaxIdleTimeout) + s.keepAliveInterval = utils.MinDuration(s.idleTimeout/2, protocol.MaxKeepAliveInterval) if err := s.streamsMap.UpdateLimits(params); err != nil { s.closeLocal(err) return diff --git a/session_test.go b/session_test.go index 4602dbb07..ff65253ad 100644 --- a/session_test.go +++ b/session_test.go @@ -132,6 +132,7 @@ var _ = Describe("Session", func() { cryptoSetup = mocks.NewMockCryptoSetup(mockCtrl) sess.cryptoStreamHandler = cryptoSetup sess.handshakeComplete = true + sess.idleTimeout = time.Hour }) AfterEach(func() { @@ -1326,7 +1327,7 @@ var _ = Describe("Session", func() { sess.run() }() params := &handshake.TransportParameters{ - IdleTimeout: 90 * time.Second, + MaxIdleTimeout: 90 * time.Second, InitialMaxStreamDataBidiLocal: 0x5000, InitialMaxData: 0x5000, ActiveConnectionIDLimit: 3, @@ -1357,7 +1358,7 @@ var _ = Describe("Session", func() { Context("keep-alives", func() { setRemoteIdleTimeout := func(t time.Duration) { - tp := &handshake.TransportParameters{IdleTimeout: t} + tp := &handshake.TransportParameters{MaxIdleTimeout: t} streamManager.EXPECT().UpdateLimits(gomock.Any()) packer.EXPECT().HandleTransportParameters(gomock.Any()) sess.processTransportParameters(tp.Marshal()) @@ -1372,6 +1373,7 @@ var _ = Describe("Session", func() { } BeforeEach(func() { + sess.config.MaxIdleTimeout = 30 * time.Second sess.config.KeepAlive = true }) @@ -1387,7 +1389,7 @@ var _ = Describe("Session", func() { It("sends a PING as a keep-alive after half the idle timeout", func() { setRemoteIdleTimeout(5 * time.Second) - sess.lastPacketReceivedTime = time.Now().Add(-protocol.MaxKeepAliveInterval) + sess.lastPacketReceivedTime = time.Now().Add(-5 * time.Second / 2) sent := make(chan struct{}) packer.EXPECT().PackPacket().Do(func() (*packedPacket, error) { close(sent) @@ -1398,6 +1400,7 @@ var _ = Describe("Session", func() { }) It("sends a PING after a maximum of protocol.MaxKeepAliveInterval", func() { + sess.config.MaxIdleTimeout = time.Hour setRemoteIdleTimeout(time.Hour) sess.lastPacketReceivedTime = time.Now().Add(-protocol.MaxKeepAliveInterval).Add(-time.Millisecond) sent := make(chan struct{}) @@ -1471,7 +1474,7 @@ var _ = Describe("Session", func() { It("does not use the idle timeout before the handshake complete", func() { sess.handshakeComplete = false - sess.config.IdleTimeout = 9999 * time.Second + sess.config.MaxIdleTimeout = 9999 * time.Second sess.lastPacketReceivedTime = time.Now().Add(-time.Minute) packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(f *wire.ConnectionCloseFrame) (*packedPacket, error) { Expect(f.ErrorCode).To(Equal(qerr.NoError)) @@ -1500,7 +1503,7 @@ var _ = Describe("Session", func() { sessionRunner.EXPECT().Remove(gomock.Any()), ) cryptoSetup.EXPECT().Close() - sess.config.IdleTimeout = 0 + sess.idleTimeout = 0 done := make(chan struct{}) go func() { defer GinkgoRecover() @@ -1519,7 +1522,7 @@ var _ = Describe("Session", func() { It("doesn't time out when it just sent a packet", func() { sess.lastPacketReceivedTime = time.Now().Add(-time.Hour) sess.firstAckElicitingPacketAfterIdleSentTime = time.Now().Add(-time.Second) - sess.config.IdleTimeout = 30 * time.Second + sess.idleTimeout = 30 * time.Second go func() { defer GinkgoRecover() cryptoSetup.EXPECT().RunHandshake().MaxTimes(1) @@ -1842,6 +1845,16 @@ var _ = Describe("Client Session", func() { Eventually(sess.Context().Done()).Should(BeClosed()) }) + It("uses the minimum of the peers' idle timeouts", func() { + sess.config.MaxIdleTimeout = 19 * time.Second + params := &handshake.TransportParameters{ + MaxIdleTimeout: 18 * time.Second, + } + packer.EXPECT().HandleTransportParameters(gomock.Any()) + sess.processTransportParameters(params.Marshal()) + Expect(sess.idleTimeout).To(Equal(18 * time.Second)) + }) + It("errors if the TransportParameters contain an original_connection_id, although no Retry was performed", func() { params := &handshake.TransportParameters{ OriginalConnectionID: protocol.ConnectionID{0xde, 0xca, 0xfb, 0xad},