From c6d42b10936d6b5d7729a2171b8ec98813bc2bc8 Mon Sep 17 00:00:00 2001
From: Andrew Dunham <andrew@du.nham.ca>
Date: Tue, 14 May 2024 12:28:01 -0400
Subject: [PATCH] derp: remove stats goroutine, use a timer

Without changing behaviour, don't create a goroutine per connection that
sits and sleeps, but rather use a timer that wakes up and gathers
statistics on a regular basis.

Fixes #12127

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: Ibc486447e403070bdc3c2cd8ae340e7d02854f21
---
 derp/derp_server.go         |  3 ++-
 derp/derp_server_default.go |  5 +++--
 derp/derp_server_linux.go   | 37 ++++++++++++++++++++-----------------
 3 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/derp/derp_server.go b/derp/derp_server.go
index b5085ff10..115dbf761 100644
--- a/derp/derp_server.go
+++ b/derp/derp_server.go
@@ -776,7 +776,6 @@ func (c *sclient) run(ctx context.Context) error {
 	var grp errgroup.Group
 	sendCtx, cancelSender := context.WithCancel(ctx)
 	grp.Go(func() error { return c.sendLoop(sendCtx) })
-	grp.Go(func() error { return c.statsLoop(sendCtx) })
 	defer func() {
 		cancelSender()
 		if err := grp.Wait(); err != nil && !c.s.isClosed() {
@@ -788,6 +787,8 @@ func (c *sclient) run(ctx context.Context) error {
 		}
 	}()
 
+	c.startStatsLoop(sendCtx)
+
 	for {
 		ft, fl, err := readFrameHeader(c.br)
 		c.debugLogf("read frame type %d len %d err %v", ft, fl, err)
diff --git a/derp/derp_server_default.go b/derp/derp_server_default.go
index 777768ce8..3e0b5b5e9 100644
--- a/derp/derp_server_default.go
+++ b/derp/derp_server_default.go
@@ -7,6 +7,7 @@ package derp
 
 import "context"
 
-func (c *sclient) statsLoop(ctx context.Context) error {
-	return nil
+func (c *sclient) startStatsLoop(ctx context.Context) {
+	// Nothing to do
+	return
 }
diff --git a/derp/derp_server_linux.go b/derp/derp_server_linux.go
index 48da8ed30..bfc2aade6 100644
--- a/derp/derp_server_linux.go
+++ b/derp/derp_server_linux.go
@@ -12,40 +12,43 @@ import (
 	"tailscale.com/net/tcpinfo"
 )
 
-func (c *sclient) statsLoop(ctx context.Context) error {
+func (c *sclient) startStatsLoop(ctx context.Context) {
 	// Get the RTT initially to verify it's supported.
 	conn := c.tcpConn()
 	if conn == nil {
 		c.s.tcpRtt.Add("non-tcp", 1)
-		return nil
+		return
 	}
 	if _, err := tcpinfo.RTT(conn); err != nil {
 		c.logf("error fetching initial RTT: %v", err)
 		c.s.tcpRtt.Add("error", 1)
-		return nil
+		return
 	}
 
 	const statsInterval = 10 * time.Second
 
-	ticker, tickerChannel := c.s.clock.NewTicker(statsInterval)
-	defer ticker.Stop()
+	// Don't launch a goroutine; use a timer instead.
+	var gatherStats func()
+	gatherStats = func() {
+		// Do nothing if the context is finished.
+		if ctx.Err() != nil {
+			return
+		}
 
-statsLoop:
-	for {
-		select {
-		case <-tickerChannel:
-			rtt, err := tcpinfo.RTT(conn)
-			if err != nil {
-				continue statsLoop
-			}
+		// Reschedule ourselves when this stats gathering is finished.
+		defer c.s.clock.AfterFunc(statsInterval, gatherStats)
 
-			// TODO(andrew): more metrics?
+		// Gather TCP RTT information.
+		rtt, err := tcpinfo.RTT(conn)
+		if err == nil {
 			c.s.tcpRtt.Add(durationToLabel(rtt), 1)
-
-		case <-ctx.Done():
-			return ctx.Err()
 		}
+
+		// TODO(andrew): more metrics?
 	}
+
+	// Kick off the initial timer.
+	c.s.clock.AfterFunc(statsInterval, gatherStats)
 }
 
 // tcpConn attempts to get the underlying *net.TCPConn from this client's