Cleanup

Signed-off-by: Andrea Gottardo <andrea@gottardo.me>

ipn/ipnlocal/local.go

@@ -346,10 +346,8 @@ type LocalBackend struct {
 	// refreshAutoExitNode indicates if the exit node should be recomputed when the next netcheck report is available.
 	refreshAutoExitNode bool
 
-	// captiveDetectionTimer is a timer acting as debouncer to trigger captive portal detection
+	needsCaptiveDetection chan bool
-	// upon a lack of Internet connectivity, avoiding spurious detection attempts.
+	captiveStopGoroutine  chan struct{}
-	// It is always nil unless a captive portal detection attempt is pending.
-	captiveDetectionTimer *time.Timer
 }
 
 // HealthTracker returns the health tracker for the backend.
@@ -425,6 +423,8 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 		clock:                 clock,
 		selfUpdateProgress:    make([]ipnstate.UpdateProgress, 0),
 		lastSelfUpdateState:   ipnstate.UpdateFinished,
+		needsCaptiveDetection: make(chan bool),
+		captiveStopGoroutine:  make(chan struct{}),
 	}
 	mConn.SetNetInfoCallback(b.setNetInfo)
 
@@ -739,19 +739,10 @@ func (b *LocalBackend) onHealthChange(w *health.Warnable, us *health.UnhealthySt
 	}
 
 	if isConnectivityImpacted {
-		b.logf("health: connectivity impacted; triggering captive portal detection in %v", captivePortalDetectionInterval)
+		b.logf("health: connectivity impacted; triggering captive portal detection")
-		b.mu.Lock()
+		b.needsCaptiveDetection <- true
-		if b.captiveDetectionTimer != nil {
-			b.captiveDetectionTimer.Reset(captivePortalDetectionInterval)
 	} else {
-			b.captiveDetectionTimer = time.AfterFunc(captivePortalDetectionInterval, func() {
+		b.needsCaptiveDetection <- false
-				b.mu.Lock()
-				b.captiveDetectionTimer = nil
-				b.mu.Unlock()
-				b.performCaptiveDetection()
-			})
-		}
-		b.mu.Unlock()
 	}
 }
 
@@ -2141,18 +2132,68 @@ var captivePortalWarnable = health.Register(&health.Warnable{
 	ImpactsConnectivity: true,
 })
 
+func (b *LocalBackend) checkCaptivePortalLoop() {
+	var tmr *time.Timer
+	for {
+		// First, see if we have a signal on our "healthy" channel, which
+		// takes priority over an existing timer.
+		select {
+		case needsCaptiveDetection := <-b.needsCaptiveDetection:
+			if !needsCaptiveDetection && tmr != nil {
+				println("checkCaptivePortalLoop: canceling existing timer (early)")
+				if !tmr.Stop() {
+					<-tmr.C
+				}
+				tmr = nil
+			}
+		default:
+		}
+
+		var timerChan <-chan time.Time
+		if tmr != nil {
+			timerChan = tmr.C
+		}
+		select {
+		case <-b.captiveStopGoroutine:
+			// All done; stop the timer and then exit.
+			if tmr != nil && !tmr.Stop() {
+				<-tmr.C
+			}
+			println("checkCaptivePortalLoop: shutting down")
+			return
+		case <-timerChan:
+			// Kick off captive portal check
+			println("checkCaptivePortalLoop: will do captive portal check")
+			b.performCaptiveDetection()
+			// nil out timer to force recreation
+			tmr = nil
+		case needsCaptiveDetection := <-b.needsCaptiveDetection:
+			if needsCaptiveDetection {
+				// If there's an existing timer, nothing to do; just
+				// continue waiting for it to expire. Otherwise, create
+				// a new timer.
+				if tmr == nil {
+					tmr = time.NewTimer(2 * time.Second)
+					println("checkCaptivePortalLoop: started new timer")
+				}
+			} else {
+				// Healthy; cancel any existing timer
+				if tmr != nil && !tmr.Stop() {
+					<-tmr.C
+				}
+				if tmr != nil {
+					println("checkCaptivePortalLoop: canceling existing timer")
+				}
+				tmr = nil
+			}
+		}
+	}
+}
+
 // performCaptiveDetection checks if captive portal detection is enabled via controlknob. If so, it runs
 // the detection and updates the Warnable accordingly.
 func (b *LocalBackend) performCaptiveDetection() {
-	captiveDetectionDisabledByControlKnob := b.ControlKnobs().DisableCaptivePortalDetection.Load()
+	if !b.shouldRunCaptivePortalDetection() {
-	if captiveDetectionDisabledByControlKnob {
-		b.logf("performCaptiveDetection: disabled by controlknob")
-		return
-	}
-
-	// Only perform detection if ipn.State is Running.
-	if b.State() != ipn.Running {
-		b.logf("performCaptiveDetection: ignored because not running")
 		return
 	}
 
@@ -2168,8 +2209,10 @@ func (b *LocalBackend) performCaptiveDetection() {
 			preferredDERP = b.hostinfo.NetInfo.PreferredDERP
 		}
 	}
+	ctx := b.ctx
+	netMon := b.NetMon()
 	b.mu.Unlock()
-	found := d.Detect(b.ctx, b.NetMon(), dm, preferredDERP)
+	found := d.Detect(ctx, netMon, dm, preferredDERP)
 	if found {
 		b.health.SetUnhealthy(captivePortalWarnable, health.Args{})
 	} else {
@@ -2177,6 +2220,15 @@ func (b *LocalBackend) performCaptiveDetection() {
 	}
 }
 
+// shouldRunCaptivePortalDetection reports whether captive portal detection
+// should be run. It is enabled by default, but can be disabled via a control
+// knob. It is also only run when the backend is in a Running state.
+func (b *LocalBackend) shouldRunCaptivePortalDetection() bool {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	return !b.ControlKnobs().DisableCaptivePortalDetection.Load() && b.State() == ipn.Running
+}
+
 // packetFilterPermitsUnlockedNodes reports any peer in peers with the
 // UnsignedPeerAPIOnly bool set true has any of its allowed IPs in the packet
 // filter.
@@ -4570,13 +4622,11 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State, unlock unlock
 	if newState == ipn.Running {
 		b.authURL = ""
 		b.authURLTime = time.Time{}
+		go b.checkCaptivePortalLoop()
 	} else if oldState == ipn.Running {
 		// Transitioning away from running.
 		b.closePeerAPIListenersLocked()
-		if b.captiveDetectionTimer != nil {
+		close(b.captiveStopGoroutine)
-			b.captiveDetectionTimer.Stop()
-			b.captiveDetectionTimer = nil
-		}
 	}
 	b.pauseOrResumeControlClientLocked()
 

net/captivedetection/captivedetection.go

@@ -96,7 +96,7 @@ func (d *Detector) detectCaptivePortalWithGOOS(ctx context.Context, netMon *netm
 			continue
 		}
 		d.logf("[v2] attempting to do captive portal detection on interface %s", ifName)
-		res := d.detectOnInterface(ctx, i.Index, endpoints, netMon)
+		res := d.detectOnInterface(ctx, i.Index, endpoints)
 		if res {
 			d.logf("DetectCaptivePortal(found=true,ifName=%s)", found, ifName)
 			return true
@@ -128,7 +128,7 @@ func interfaceNameDoesNotNeedCaptiveDetection(ifName string, goos string) bool {
 // return a "204 No Content" response and checking if that's what we get.
 //
 // The boolean return is whether we think we have a captive portal.
-func (d *Detector) detectOnInterface(ctx context.Context, ifIndex int, endpoints []Endpoint, netMon *netmon.Monitor) bool {
+func (d *Detector) detectOnInterface(ctx context.Context, ifIndex int, endpoints []Endpoint) bool {
 	defer d.httpClient.CloseIdleConnections()
 
 	d.logf("[v2] %d available captive portal detection endpoints: %v", len(endpoints), endpoints)