introduce a quic.Config.HandshakeIdleTimeout, remove HandshakeTimeout

Changelog.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## v0.20.0 (unreleased)
+
+- Remove the `quic.Config.HandshakeTimeout`. Introduce a `quic.Config.HandshakeIdleTimeout`.
+
 ## v0.17.1 (2020-06-20)
 
 - Supports QUIC WG draft-29.

client_test.go

@@ -140,7 +140,7 @@ var _ = Describe("Client", func() {
 				sess.EXPECT().HandshakeComplete().Return(context.Background())
 				return sess
 			}
-			_, err := DialAddr("localhost:17890", tlsConf, &Config{HandshakeTimeout: time.Millisecond})
+			_, err := DialAddr("localhost:17890", tlsConf, &Config{HandshakeIdleTimeout: time.Millisecond})
 			Expect(err).ToNot(HaveOccurred())
 			Eventually(remoteAddrChan).Should(Receive(Equal("127.0.0.1:17890")))
 		})
@@ -458,7 +458,7 @@ var _ = Describe("Client", func() {
 			It("setups with the right values", func() {
 				tokenStore := NewLRUTokenStore(10, 4)
 				config := &Config{
-					HandshakeTimeout:      1337 * time.Minute,
+					HandshakeIdleTimeout:  1337 * time.Minute,
 					MaxIdleTimeout:        42 * time.Hour,
 					MaxIncomingStreams:    1234,
 					MaxIncomingUniStreams: 4321,
@@ -467,7 +467,7 @@ var _ = Describe("Client", func() {
 					TokenStore:            tokenStore,
 				}
 				c := populateClientConfig(config, false)
-				Expect(c.HandshakeTimeout).To(Equal(1337 * time.Minute))
+				Expect(c.HandshakeIdleTimeout).To(Equal(1337 * time.Minute))
 				Expect(c.MaxIdleTimeout).To(Equal(42 * time.Hour))
 				Expect(c.MaxIncomingStreams).To(BeEquivalentTo(1234))
 				Expect(c.MaxIncomingUniStreams).To(BeEquivalentTo(4321))
@@ -513,7 +513,7 @@ var _ = Describe("Client", func() {
 			It("fills in default values if options are not set in the Config", func() {
 				c := populateClientConfig(&Config{}, false)
 				Expect(c.Versions).To(Equal(protocol.SupportedVersions))
-				Expect(c.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout))
+				Expect(c.HandshakeIdleTimeout).To(Equal(protocol.DefaultHandshakeIdleTimeout))
 				Expect(c.MaxIdleTimeout).To(Equal(protocol.DefaultIdleTimeout))
 			})
 		})

config.go

@@ -2,6 +2,9 @@ package quic
 
 import (
 	"errors"
+	"time"
+
+	"github.com/lucas-clemente/quic-go/internal/utils"
 
 	"github.com/lucas-clemente/quic-go/internal/protocol"
 )
@@ -12,6 +15,10 @@ func (c *Config) Clone() *Config {
 	return &copy
 }
 
+func (c *Config) handshakeTimeout() time.Duration {
+	return utils.MaxDuration(protocol.DefaultHandshakeTimeout, 2*c.HandshakeIdleTimeout)
+}
+
 func validateConfig(config *Config) error {
 	if config == nil {
 		return nil
@@ -56,9 +63,9 @@ func populateConfig(config *Config) *Config {
 	if len(versions) == 0 {
 		versions = protocol.SupportedVersions
 	}
-	handshakeTimeout := protocol.DefaultHandshakeTimeout
-	if config.HandshakeTimeout != 0 {
-		handshakeTimeout = config.HandshakeTimeout
+	handshakeIdleTimeout := protocol.DefaultHandshakeIdleTimeout
+	if config.HandshakeIdleTimeout != 0 {
+		handshakeIdleTimeout = config.HandshakeIdleTimeout
 	}
 	idleTimeout := protocol.DefaultIdleTimeout
 	if config.MaxIdleTimeout != 0 {
@@ -87,7 +94,7 @@ func populateConfig(config *Config) *Config {
 
 	return &Config{
 		Versions:                              versions,
-		HandshakeTimeout:                      handshakeTimeout,
+		HandshakeIdleTimeout:                  handshakeIdleTimeout,
 		MaxIdleTimeout:                        idleTimeout,
 		AcceptToken:                           config.AcceptToken,
 		KeepAlive:                             config.KeepAlive,

config_test.go

@@ -51,7 +51,7 @@ var _ = Describe("Config", func() {
 				f.Set(reflect.ValueOf([]VersionNumber{1, 2, 3}))
 			case "ConnectionIDLength":
 				f.Set(reflect.ValueOf(8))
-			case "HandshakeTimeout":
+			case "HandshakeIdleTimeout":
 				f.Set(reflect.ValueOf(time.Second))
 			case "MaxIdleTimeout":
 				f.Set(reflect.ValueOf(time.Hour))
@@ -77,6 +77,17 @@ var _ = Describe("Config", func() {
 		}
 		return c
 	}
+
+	It("uses 10s handshake timeout for short handshake idle timeouts", func() {
+		c := &Config{HandshakeIdleTimeout: time.Second}
+		Expect(c.handshakeTimeout()).To(Equal(protocol.DefaultHandshakeTimeout))
+	})
+
+	It("uses twice the handshake idle timeouts for the handshake timeout, for long handshake idle timeouts", func() {
+		c := &Config{HandshakeIdleTimeout: time.Second * 11 / 2}
+		Expect(c.handshakeTimeout()).To(Equal(11 * time.Second))
+	})
+
 	Context("cloning", func() {
 		It("clones function fields", func() {
 			var calledAcceptToken bool
@@ -126,7 +137,7 @@ var _ = Describe("Config", func() {
 		It("populates empty fields with default values", func() {
 			c := populateConfig(&Config{})
 			Expect(c.Versions).To(Equal(protocol.SupportedVersions))
-			Expect(c.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout))
+			Expect(c.HandshakeIdleTimeout).To(Equal(protocol.DefaultHandshakeIdleTimeout))
 			Expect(c.MaxReceiveStreamFlowControlWindow).To(BeEquivalentTo(protocol.DefaultMaxReceiveStreamFlowControlWindow))
 			Expect(c.MaxReceiveConnectionFlowControlWindow).To(BeEquivalentTo(protocol.DefaultMaxReceiveConnectionFlowControlWindow))
 			Expect(c.MaxIncomingStreams).To(BeEquivalentTo(protocol.DefaultMaxIncomingStreams))

http3/roundtrip_test.go

@@ -109,7 +109,7 @@ var _ = Describe("RoundTripper", func() {
 		})
 
 		It("uses the quic.Config, if provided", func() {
-			config := &quic.Config{HandshakeTimeout: time.Millisecond}
+			config := &quic.Config{HandshakeIdleTimeout: time.Millisecond}
 			var receivedConfig *quic.Config
 			dialAddr = func(addr string, tlsConf *tls.Config, config *quic.Config) (quic.EarlySession, error) {
 				receivedConfig = config
@@ -118,7 +118,7 @@ var _ = Describe("RoundTripper", func() {
 			rt.QuicConfig = config
 			_, err := rt.RoundTrip(req1)
 			Expect(err).To(MatchError("handshake error"))
-			Expect(receivedConfig.HandshakeTimeout).To(Equal(config.HandshakeTimeout))
+			Expect(receivedConfig.HandshakeIdleTimeout).To(Equal(config.HandshakeIdleTimeout))
 		})
 
 		It("uses the custom dialer, if provided", func() {

http3/server_test.go

@@ -522,7 +522,7 @@ var _ = Describe("Server", func() {
 		}
 
 		It("uses the quic.Config to start the QUIC server", func() {
-			conf := &quic.Config{HandshakeTimeout: time.Nanosecond}
+			conf := &quic.Config{HandshakeIdleTimeout: time.Nanosecond}
 			var receivedConf *quic.Config
 			quicListenAddr = func(addr string, _ *tls.Config, config *quic.Config) (quic.EarlyListener, error) {
 				receivedConf = config

integrationtests/self/handshake_drop_test.go

@@ -38,7 +38,7 @@ var _ = Describe("Handshake drop tests", func() {
 	startListenerAndProxy := func(dropCallback quicproxy.DropCallback, doRetry bool, longCertChain bool, version protocol.VersionNumber) {
 		conf := getQuicConfig(&quic.Config{
 			MaxIdleTimeout:       timeout,
-			HandshakeTimeout: timeout,
+			HandshakeIdleTimeout: timeout,
 			Versions:             []protocol.VersionNumber{version},
 		})
 		if !doRetry {
@@ -89,7 +89,7 @@ var _ = Describe("Handshake drop tests", func() {
 				getTLSClientConfig(),
 				getQuicConfig(&quic.Config{
 					MaxIdleTimeout:       timeout,
-					HandshakeTimeout: timeout,
+					HandshakeIdleTimeout: timeout,
 					Versions:             []protocol.VersionNumber{version},
 				}),
 			)
@@ -127,7 +127,7 @@ var _ = Describe("Handshake drop tests", func() {
 				getTLSClientConfig(),
 				getQuicConfig(&quic.Config{
 					MaxIdleTimeout:       timeout,
-					HandshakeTimeout: timeout,
+					HandshakeIdleTimeout: timeout,
 					Versions:             []protocol.VersionNumber{version},
 				}),
 			)
@@ -160,7 +160,7 @@ var _ = Describe("Handshake drop tests", func() {
 				getTLSClientConfig(),
 				getQuicConfig(&quic.Config{
 					MaxIdleTimeout:       timeout,
-					HandshakeTimeout: timeout,
+					HandshakeIdleTimeout: timeout,
 					Versions:             []protocol.VersionNumber{version},
 				}),
 			)

integrationtests/self/handshake_rtt_test.go

@@ -145,7 +145,7 @@ var _ = Describe("Handshake RTT tests", func() {
 		serverConfig.AcceptToken = func(_ net.Addr, _ *quic.Token) bool {
 			return false
 		}
-		clientConfig.HandshakeTimeout = 500 * time.Millisecond
+		clientConfig.HandshakeIdleTimeout = 500 * time.Millisecond
 		runServerAndProxy()
 		_, err := quic.DialAddr(
 			fmt.Sprintf("localhost:%d", proxy.LocalAddr().(*net.UDPAddr).Port),
@@ -153,6 +153,8 @@ var _ = Describe("Handshake RTT tests", func() {
 			clientConfig,
 		)
 		Expect(err).To(HaveOccurred())
-		Expect(err.Error()).To(ContainSubstring("Handshake did not complete in time"))
+		nerr, ok := err.(net.Error)
+		Expect(ok).To(BeTrue())
+		Expect(nerr.Timeout()).To(BeTrue())
 	})
 })

integrationtests/self/mitm_test.go

@@ -338,7 +338,7 @@ var _ = Describe("MITM test", func() {
 						getQuicConfig(&quic.Config{
 							Versions:             []protocol.VersionNumber{version},
 							ConnectionIDLength:   connIDLen,
-							HandshakeTimeout:   2 * time.Second,
+							HandshakeIdleTimeout: 2 * time.Second,
 						}),
 					)
 					return err

integrationtests/self/timeout_test.go

@@ -66,7 +66,7 @@ var _ = Describe("Timeout tests", func() {
 			_, err := quic.DialAddr(
 				"localhost:12345",
 				getTLSClientConfig(),
-				getQuicConfig(&quic.Config{HandshakeTimeout: 10 * time.Millisecond}),
+				getQuicConfig(&quic.Config{HandshakeIdleTimeout: 10 * time.Millisecond}),
 			)
 			errChan <- err
 		}()
@@ -431,7 +431,7 @@ var _ = Describe("Timeout tests", func() {
 					fmt.Sprintf("localhost:%d", ln.Addr().(*net.UDPAddr).Port),
 					getTLSClientConfig(),
 					getQuicConfig(&quic.Config{
-						HandshakeTimeout: handshakeTimeout,
+						HandshakeIdleTimeout: handshakeTimeout,
 						MaxIdleTimeout:       handshakeTimeout,
 					}),
 				)
@@ -464,7 +464,7 @@ var _ = Describe("Timeout tests", func() {
 				"localhost:0",
 				getTLSConfig(),
 				getQuicConfig(&quic.Config{
-					HandshakeTimeout: handshakeTimeout,
+					HandshakeIdleTimeout: handshakeTimeout,
 					MaxIdleTimeout:       handshakeTimeout,
 					KeepAlive:            true,
 				}),

interface.go

@@ -217,10 +217,10 @@ type Config struct {
 	// If used for a server, or dialing on a packet conn, a 4 byte connection ID will be used.
 	// When dialing on a packet conn, the ConnectionIDLength value must be the same for every Dial call.
 	ConnectionIDLength int
-	// HandshakeTimeout is the maximum duration that the cryptographic handshake may take.
-	// If the timeout is exceeded, the connection is closed.
-	// If this value is zero, the timeout is set to 10 seconds.
-	HandshakeTimeout time.Duration
+	// HandshakeIdleTimeout is the idle timeout before completion of the handshake.
+	// Specifically, if we don't receive any packet from the peer within this time, the connection attempt is aborted.
+	// If this value is zero, the timeout is set to 5 seconds.
+	HandshakeIdleTimeout time.Duration
 	// 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.

internal/protocol/params.go

@@ -98,6 +98,9 @@ const MinRemoteIdleTimeout = 5 * time.Second
 // DefaultIdleTimeout is the default idle timeout
 const DefaultIdleTimeout = 30 * time.Second
 
+// DefaultHandshakeIdleTimeout is the default idle timeout used before handshake completion.
+const DefaultHandshakeIdleTimeout = 5 * time.Second
+
 // DefaultHandshakeTimeout is the default timeout for a connection until the crypto handshake succeeds.
 const DefaultHandshakeTimeout = 10 * time.Second
 

server_test.go

@@ -124,7 +124,7 @@ var _ = Describe("Server", func() {
 		Expect(err).ToNot(HaveOccurred())
 		server := ln.(*baseServer)
 		Expect(server.config.Versions).To(Equal(protocol.SupportedVersions))
-		Expect(server.config.HandshakeTimeout).To(Equal(protocol.DefaultHandshakeTimeout))
+		Expect(server.config.HandshakeIdleTimeout).To(Equal(protocol.DefaultHandshakeIdleTimeout))
 		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())
@@ -138,7 +138,7 @@ var _ = Describe("Server", func() {
 		config := Config{
 			Versions:             supportedVersions,
 			AcceptToken:          acceptToken,
-			HandshakeTimeout:  1337 * time.Hour,
+			HandshakeIdleTimeout: 1337 * time.Hour,
 			MaxIdleTimeout:       42 * time.Minute,
 			KeepAlive:            true,
 			StatelessResetKey:    []byte("foobar"),
@@ -148,7 +148,7 @@ var _ = Describe("Server", func() {
 		server := ln.(*baseServer)
 		Expect(server.sessionHandler).ToNot(BeNil())
 		Expect(server.config.Versions).To(Equal(supportedVersions))
-		Expect(server.config.HandshakeTimeout).To(Equal(1337 * time.Hour))
+		Expect(server.config.HandshakeIdleTimeout).To(Equal(1337 * time.Hour))
 		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())

session.go

@@ -587,19 +587,23 @@ runLoop:
 			s.logger.Debugf("Sending a keep-alive PING to keep the connection alive.")
 			s.framer.QueueControlFrame(&wire.PingFrame{})
 			s.keepAlivePingSent = true
-		} else if !s.handshakeComplete && now.Sub(s.sessionCreationTime) >= s.config.HandshakeTimeout {
+		} 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.NewTimeoutError("Handshake did not complete in time"))
 			continue
-		} else if s.handshakeComplete && now.Sub(s.idleTimeoutStartTime()) >= s.idleTimeout {
+		} 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.NewTimeoutError("No recent network activity"))
 				continue
 			}
+		}
 
 		if err := s.sendPackets(); err != nil {
 			s.closeLocal(err)
@@ -646,7 +650,7 @@ func (s *session) nextKeepAliveTime() time.Time {
 func (s *session) maybeResetTimer() {
 	var deadline time.Time
 	if !s.handshakeComplete {
-		deadline = s.sessionCreationTime.Add(s.config.HandshakeTimeout)
+		deadline = s.sessionCreationTime.Add(utils.MinDuration(s.config.handshakeTimeout(), s.config.HandshakeIdleTimeout))
 	} else {
 		if keepAliveTime := s.nextKeepAliveTime(); !keepAliveTime.IsZero() {
 			deadline = keepAliveTime

session_test.go

@@ -1978,6 +1978,7 @@ var _ = Describe("Session", func() {
 
 		It("does not use the idle timeout before the handshake complete", func() {
 			sess.handshakeComplete = false
+			sess.config.HandshakeIdleTimeout = 9999 * time.Second
 			sess.config.MaxIdleTimeout = 9999 * time.Second
 			sess.lastPacketReceivedTime = time.Now().Add(-time.Minute)
 			packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(quicErr *qerr.QuicError) (*coalescedPacket, error) {
@@ -2007,6 +2008,35 @@ var _ = Describe("Session", func() {
 			Eventually(sess.Context().Done()).Should(BeClosed())
 		})
 
+		It("closes the session due to the idle timeout before handshake", func() {
+			sess.config.HandshakeIdleTimeout = 0
+			packer.EXPECT().PackCoalescedPacket().AnyTimes()
+			sessionRunner.EXPECT().Remove(gomock.Any()).AnyTimes()
+			cryptoSetup.EXPECT().Close()
+			gomock.InOrder(
+				tracer.EXPECT().ClosedConnection(gomock.Any()).Do(func(reason logging.CloseReason) {
+					timeout, ok := reason.Timeout()
+					Expect(ok).To(BeTrue())
+					Expect(timeout).To(Equal(logging.TimeoutReasonIdle))
+				}),
+				tracer.EXPECT().Close(),
+			)
+			done := make(chan struct{})
+			sess.handshakeComplete = false
+			go func() {
+				defer GinkgoRecover()
+				cryptoSetup.EXPECT().RunHandshake().MaxTimes(1)
+				cryptoSetup.EXPECT().GetSessionTicket().MaxTimes(1)
+				err := sess.run()
+				nerr, ok := err.(net.Error)
+				Expect(ok).To(BeTrue())
+				Expect(nerr.Timeout()).To(BeTrue())
+				Expect(err.Error()).To(ContainSubstring("No recent network activity"))
+				close(done)
+			}()
+			Eventually(done).Should(BeClosed())
+		})
+
 		It("closes the session due to the idle timeout after handshake", func() {
 			packer.EXPECT().PackCoalescedPacket().AnyTimes()
 			gomock.InOrder(