From 6407f5bf680283bf7e3755976306767da2c55e66 Mon Sep 17 00:00:00 2001
From: Luke Tucker <voxluci@gmail.com>
Date: Sun, 26 Jan 2020 23:13:54 -0500
Subject: [PATCH] Fix keepalive ping (#2316)

The firstAckElicitingPacketAfterIdleSendTime condition was inverted
in a recent PR, maybe just a typo.  This was causing only one ping
to be sent during periods of no activity.  The ack from the first
keepalive ping causes firstAckElicitingPacketAfterIdleSentTime
to be set to zero.  If there is no further activity, it will remain
zero and prevent further keepalive pings.
---
 integrationtests/self/timeout_test.go | 60 +++++++++++++++++++++++++++
 session.go                            |  2 +-
 session_test.go                       |  2 -
 3 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/integrationtests/self/timeout_test.go b/integrationtests/self/timeout_test.go
index fd05721f3..53a51c80d 100644
--- a/integrationtests/self/timeout_test.go
+++ b/integrationtests/self/timeout_test.go
@@ -229,4 +229,64 @@ var _ = Describe("Timeout tests", func() {
 			Eventually(serverSessionClosed).Should(BeClosed())
 		})
 	})
+
+	It("does not time out if keepalive is set", func() {
+		const idleTimeout = 100 * time.Millisecond
+
+		server, err := quic.ListenAddr(
+			"localhost:0",
+			getTLSConfig(),
+			nil,
+		)
+		Expect(err).ToNot(HaveOccurred())
+		defer server.Close()
+
+		serverSessionClosed := make(chan struct{})
+		go func() {
+			defer GinkgoRecover()
+			sess, err := server.Accept(context.Background())
+			Expect(err).ToNot(HaveOccurred())
+			sess.AcceptStream(context.Background()) // blocks until the session is closed
+			close(serverSessionClosed)
+		}()
+
+		drop := utils.AtomicBool{}
+
+		proxy, err := quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{
+			RemoteAddr: fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
+			DropPacket: func(quicproxy.Direction, []byte) bool {
+				return drop.Get()
+			},
+		})
+		Expect(err).ToNot(HaveOccurred())
+		defer proxy.Close()
+
+		sess, err := quic.DialAddr(
+			fmt.Sprintf("localhost:%d", proxy.LocalPort()),
+			getTLSClientConfig(),
+			&quic.Config{
+				MaxIdleTimeout: idleTimeout,
+				KeepAlive:      true,
+			},
+		)
+		Expect(err).ToNot(HaveOccurred())
+
+		// wait longer than the idle timeout
+		time.Sleep(3 * idleTimeout)
+		str, err := sess.OpenUniStream()
+		Expect(err).ToNot(HaveOccurred())
+		_, err = str.Write([]byte("foobar"))
+		Expect(err).ToNot(HaveOccurred())
+		Consistently(serverSessionClosed).ShouldNot(BeClosed())
+
+		// idle timeout will still kick in if pings are dropped
+		drop.Set(true)
+		time.Sleep(2 * idleTimeout)
+		_, err = str.Write([]byte("foobar"))
+		checkTimeoutError(err)
+
+		Expect(server.Close()).To(Succeed())
+		Eventually(serverSessionClosed).Should(BeClosed())
+	})
+
 })
diff --git a/session.go b/session.go
index 8ec42116e..2d57bf286 100644
--- a/session.go
+++ b/session.go
@@ -586,7 +586,7 @@ func (s *session) ConnectionState() tls.ConnectionState {
 // Time when the next keep-alive packet should be sent.
 // It returns a zero time if no keep-alive should be sent.
 func (s *session) nextKeepAliveTime() time.Time {
-	if !s.config.KeepAlive || s.keepAlivePingSent || s.firstAckElicitingPacketAfterIdleSentTime.IsZero() {
+	if !s.config.KeepAlive || s.keepAlivePingSent || !s.firstAckElicitingPacketAfterIdleSentTime.IsZero() {
 		return time.Time{}
 	}
 	return s.lastPacketReceivedTime.Add(s.keepAliveInterval / 2)
diff --git a/session_test.go b/session_test.go
index 25530cbb2..023bf7a30 100644
--- a/session_test.go
+++ b/session_test.go
@@ -1370,7 +1370,6 @@ 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(-5 * time.Second / 2)
-			sess.firstAckElicitingPacketAfterIdleSentTime = time.Now().Add(-5 * time.Second / 2)
 			sent := make(chan struct{})
 			packer.EXPECT().PackPacket().Do(func() (*packedPacket, error) {
 				close(sent)
@@ -1384,7 +1383,6 @@ var _ = Describe("Session", func() {
 			sess.config.MaxIdleTimeout = time.Hour
 			setRemoteIdleTimeout(time.Hour)
 			sess.lastPacketReceivedTime = time.Now().Add(-protocol.MaxKeepAliveInterval).Add(-time.Millisecond)
-			sess.firstAckElicitingPacketAfterIdleSentTime = time.Now().Add(-protocol.MaxKeepAliveInterval).Add(-time.Millisecond)
 			sent := make(chan struct{})
 			packer.EXPECT().PackPacket().Do(func() (*packedPacket, error) {
 				close(sent)