Fix: don't incur stagger delay for skipped candidates

psiphon/controller.go

@@ -78,6 +78,7 @@ type Controller struct {
 	serverAffinityDoneBroadcast             chan struct{}
 	packetTunnelClient                      *tun.Client
 	packetTunnelTransport                   *PacketTunnelTransport
+	staggerMutex                            sync.Mutex
 }
 
 // NewController initializes a new controller.
@@ -1694,32 +1695,6 @@ loop:
 					}
 					timer.Stop()
 				}
-
-			} else {
-
-				// TODO: if candidates are to be skipped, this should be done
-				// _before_ the stagger.
-
-				p := controller.config.clientParameters.Get()
-				staggerPeriod := p.Duration(parameters.StaggerConnectionWorkersPeriod)
-				staggerJitter := p.Float(parameters.StaggerConnectionWorkersJitter)
-				p = nil
-
-				if staggerPeriod != 0 {
-
-					// Stagger concurrent connection workers.
-
-					timeout := prng.JitterDuration(staggerPeriod, staggerJitter)
-
-					timer := time.NewTimer(timeout)
-					select {
-					case <-timer.C:
-					case <-controller.establishCtx.Done():
-						timer.Stop()
-						break loop
-					}
-					timer.Stop()
-				}
 			}
 		}
 
@@ -1864,6 +1839,7 @@ loop:
 		// Increment establishConnectTunnelCount only after selectProtocol has
 		// succeeded to ensure InitialLimitTunnelProtocolsCandidateCount
 		// candidates use InitialLimitTunnelProtocols.
+		establishConnectTunnelCount := controller.establishConnectTunnelCount
 		controller.establishConnectTunnelCount += 1
 
 		isIntensive := protocol.TunnelProtocolIsResourceIntensive(dialParams.TunnelProtocol)
@@ -1881,6 +1857,33 @@ loop:
 
 		controller.concurrentEstablishTunnelsMutex.Unlock()
 
+		// Apply stagger only now that we're past MakeDialParameters and
+		// protocol selection logic which may have caused the candidate to be
+		// skipped. The stagger logic delays dialing, and we don't want to
+		// incur that delay that when skipping.
+		//
+		// Locking staggerMutex serializes staggers, so that multiple workers
+		// don't simply sleep in parallel.
+		//
+		// The stagger is applied when establishConnectTunnelCount > 0 -- that
+		// is, for all but the first dial.
+
+		p := controller.config.clientParameters.Get()
+		staggerPeriod := p.Duration(parameters.StaggerConnectionWorkersPeriod)
+		staggerJitter := p.Float(parameters.StaggerConnectionWorkersJitter)
+		p = nil
+
+		if establishConnectTunnelCount > 0 && staggerPeriod != 0 {
+			controller.staggerMutex.Lock()
+			timer := time.NewTimer(prng.JitterDuration(staggerPeriod, staggerJitter))
+			select {
+			case <-timer.C:
+			case <-controller.establishCtx.Done():
+			}
+			timer.Stop()
+			controller.staggerMutex.Unlock()
+		}
+
 		// ConnectTunnel will allocate significant memory, so first attempt to
 		// reclaim as much as possible.
 		DoGarbageCollection()

psiphon/tunnel.go

@@ -528,6 +528,13 @@ func dialTunnel(
 	sessionId string,
 	dialParams *DialParameters) (*dialResult, error) {
 
+	// Return immediately when overall context is canceled or timed-out. This
+	// avoids notice noise.
+	err := ctx.Err()
+	if err != nil {
+		return nil, common.ContextError(err)
+	}
+
 	p := config.clientParameters.Get()
 	timeout := p.Duration(parameters.TunnelConnectTimeout)
 	rateLimits := p.RateLimits(parameters.TunnelRateLimits)
@@ -566,7 +573,6 @@ func dialTunnel(
 	// Create the base transport: meek or direct connection
 
 	var dialConn net.Conn
-	var err error
 
 	if protocol.TunnelProtocolUsesMeek(dialParams.TunnelProtocol) {