From ef55a44bdb32c6c479965ed01e3a3283beda66b0 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 8 Jan 2019 14:00:03 +0700 Subject: [PATCH] restart the idle timeout when sending the first retransmittable packet --- packet_packer.go | 4 ++++ session.go | 29 ++++++++++++++++++++--------- session_test.go | 31 +++++++++++++++++++++++++------ 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index bb791838..7fae340e 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -47,6 +47,10 @@ func (p *packedPacket) EncryptionLevel() protocol.EncryptionLevel { } } +func (p *packedPacket) IsRetransmittable() bool { + return ackhandler.HasRetransmittableFrames(p.frames) +} + func (p *packedPacket) ToAckHandlerPacket() *ackhandler.Packet { return &ackhandler.Packet{ PacketNumber: p.header.PacketNumber, diff --git a/session.go b/session.go index 81f812fb..6766c6d3 100644 --- a/session.go +++ b/session.go @@ -124,8 +124,11 @@ type session struct { receivedFirstPacket bool receivedFirstForwardSecurePacket bool - sessionCreationTime time.Time - lastNetworkActivityTime time.Time + sessionCreationTime time.Time + // The idle timeout is set based on the max of the time we received the last packet... + lastPacketReceivedTime time.Time + // ... and the time we sent a new retransmittable packet after receiving a packet. + firstRetransmittablePacketAfterIdleSentTime time.Time // pacingDeadline is the time when the next packet should be sent pacingDeadline time.Time @@ -319,7 +322,7 @@ func (s *session) postSetup() error { s.timer = utils.NewTimer() now := time.Now() - s.lastNetworkActivityTime = now + s.lastPacketReceivedTime = now s.sessionCreationTime = now s.windowUpdateQueue = newWindowUpdateQueue(s.streamsMap, s.connFlowController, s.framer.QueueControlFrame) @@ -396,7 +399,7 @@ runLoop: if s.pacingDeadline.IsZero() { // the timer didn't have a pacing deadline set pacingDeadline = s.sentPacketHandler.TimeUntilSend() } - if s.config.KeepAlive && !s.keepAlivePingSent && s.handshakeComplete && time.Since(s.lastNetworkActivityTime) >= s.peerParams.IdleTimeout/2 { + if s.config.KeepAlive && !s.keepAlivePingSent && s.handshakeComplete && s.firstRetransmittablePacketAfterIdleSentTime.IsZero() && time.Since(s.lastPacketReceivedTime) >= s.peerParams.IdleTimeout/2 { // send a PING frame since there is no activity in the session s.logger.Debugf("Sending a keep-alive ping to keep the connection alive.") s.framer.QueueControlFrame(&wire.PingFrame{}) @@ -413,7 +416,7 @@ runLoop: s.destroy(qerr.TimeoutError("Handshake did not complete in time")) continue } - if s.handshakeComplete && now.Sub(s.lastNetworkActivityTime) >= s.config.IdleTimeout { + if s.handshakeComplete && now.Sub(s.idleTimeoutStartTime()) >= s.config.IdleTimeout { s.destroy(qerr.TimeoutError("No recent network activity")) continue } @@ -441,9 +444,9 @@ func (s *session) ConnectionState() ConnectionState { func (s *session) maybeResetTimer() { var deadline time.Time if s.config.KeepAlive && s.handshakeComplete && !s.keepAlivePingSent { - deadline = s.lastNetworkActivityTime.Add(s.peerParams.IdleTimeout / 2) + deadline = s.idleTimeoutStartTime().Add(s.peerParams.IdleTimeout / 2) } else { - deadline = s.lastNetworkActivityTime.Add(s.config.IdleTimeout) + deadline = s.idleTimeoutStartTime().Add(s.config.IdleTimeout) } if ackAlarm := s.receivedPacketHandler.GetAlarmTimeout(); !ackAlarm.IsZero() { @@ -463,6 +466,10 @@ func (s *session) maybeResetTimer() { s.timer.Reset(deadline) } +func (s *session) idleTimeoutStartTime() time.Time { + return utils.MaxTime(s.lastPacketReceivedTime, s.firstRetransmittablePacketAfterIdleSentTime) +} + func (s *session) handleHandshakeComplete() { s.handshakeComplete = true s.handshakeCompleteChan = nil // prevent this case from ever being selected again @@ -622,7 +629,8 @@ func (s *session) handleUnpackedPacket(packet *unpackedPacket, rcvTime time.Time } s.receivedFirstPacket = true - s.lastNetworkActivityTime = rcvTime + s.lastPacketReceivedTime = rcvTime + s.firstRetransmittablePacketAfterIdleSentTime = time.Time{} s.keepAlivePingSent = false // The client completes the handshake first (after sending the CFIN). @@ -810,7 +818,7 @@ func (s *session) handlePathChallengeFrame(frame *wire.PathChallengeFrame) { } func (s *session) handleAckFrame(frame *wire.AckFrame, pn protocol.PacketNumber, encLevel protocol.EncryptionLevel) error { - if err := s.sentPacketHandler.ReceivedAck(frame, pn, encLevel, s.lastNetworkActivityTime); err != nil { + if err := s.sentPacketHandler.ReceivedAck(frame, pn, encLevel, s.lastPacketReceivedTime); err != nil { return err } if encLevel == protocol.Encryption1RTT { @@ -1128,6 +1136,9 @@ func (s *session) sendPacket() (bool, error) { func (s *session) sendPackedPacket(packet *packedPacket) error { defer packet.buffer.Release() + if s.firstRetransmittablePacketAfterIdleSentTime.IsZero() && packet.IsRetransmittable() { + s.firstRetransmittablePacketAfterIdleSentTime = time.Now() + } s.logPacket(packet) return s.conn.Write(packet.raw) } diff --git a/session_test.go b/session_test.go index bbb3b14c..b31f63e5 100644 --- a/session_test.go +++ b/session_test.go @@ -1232,10 +1232,10 @@ var _ = Describe("Session", func() { sess.peerParams = &handshake.TransportParameters{IdleTimeout: remoteIdleTimeout} }) - It("sends a PING", func() { + It("sends a PING as a keep-alive", func() { sess.handshakeComplete = true sess.config.KeepAlive = true - sess.lastNetworkActivityTime = time.Now().Add(-remoteIdleTimeout / 2) + sess.lastPacketReceivedTime = time.Now().Add(-remoteIdleTimeout / 2) sent := make(chan struct{}) packer.EXPECT().PackPacket().Do(func() (*packedPacket, error) { close(sent) @@ -1261,7 +1261,7 @@ var _ = Describe("Session", func() { It("doesn't send a PING packet if keep-alive is disabled", func() { sess.handshakeComplete = true sess.config.KeepAlive = false - sess.lastNetworkActivityTime = time.Now().Add(-remoteIdleTimeout / 2) + sess.lastPacketReceivedTime = time.Now().Add(-remoteIdleTimeout / 2) done := make(chan struct{}) go func() { defer GinkgoRecover() @@ -1282,7 +1282,7 @@ var _ = Describe("Session", func() { It("doesn't send a PING if the handshake isn't completed yet", func() { sess.handshakeComplete = false sess.config.KeepAlive = true - sess.lastNetworkActivityTime = time.Now().Add(-remoteIdleTimeout / 2) + sess.lastPacketReceivedTime = time.Now().Add(-remoteIdleTimeout / 2) done := make(chan struct{}) go func() { defer GinkgoRecover() @@ -1309,7 +1309,7 @@ var _ = Describe("Session", func() { It("times out due to no network activity", func() { sessionRunner.EXPECT().removeConnectionID(gomock.Any()) sess.handshakeComplete = true - sess.lastNetworkActivityTime = time.Now().Add(-time.Hour) + sess.lastPacketReceivedTime = time.Now().Add(-time.Hour) done := make(chan struct{}) cryptoSetup.EXPECT().Close() go func() { @@ -1345,7 +1345,7 @@ var _ = Describe("Session", func() { It("does not use the idle timeout before the handshake complete", func() { sess.config.IdleTimeout = 9999 * time.Second - sess.lastNetworkActivityTime = time.Now().Add(-time.Minute) + 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)) return &packedPacket{}, nil @@ -1384,6 +1384,25 @@ var _ = Describe("Session", func() { }() Eventually(done).Should(BeClosed()) }) + + It("doesn't time out when it just sent a packet", func() { + sess.handshakeComplete = true + sess.lastPacketReceivedTime = time.Now().Add(-time.Hour) + sess.firstRetransmittablePacketAfterIdleSentTime = time.Now().Add(-time.Second) + sess.config.IdleTimeout = 30 * time.Second + go func() { + defer GinkgoRecover() + cryptoSetup.EXPECT().RunHandshake().Do(func() { <-sess.Context().Done() }) + sess.run() + }() + Consistently(sess.Context().Done()).ShouldNot(BeClosed()) + // make the go routine return + packer.EXPECT().PackConnectionClose(gomock.Any()).Return(&packedPacket{}, nil) + sessionRunner.EXPECT().retireConnectionID(gomock.Any()) + cryptoSetup.EXPECT().Close() + sess.Close() + Eventually(sess.Context().Done()).Should(BeClosed()) + }) }) It("stores up to MaxSessionUnprocessedPackets packets", func(done Done) {