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

@@ -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())
+	})
+
 })

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)

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)