From 0375d0a126ac6abca4e9a28945d70b9c7df36b6c Mon Sep 17 00:00:00 2001
From: Vicent Marti <vmg@strn.cat>
Date: Wed, 12 Feb 2025 14:46:27 +0100
Subject: [PATCH] pool: add documentation

Signed-off-by: Vicent Marti <vmg@strn.cat>
---
 go/pools/smartconnpool/pool.go      | 24 +++++++++++++------
 go/pools/smartconnpool/timestamp.go | 36 +++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/go/pools/smartconnpool/pool.go b/go/pools/smartconnpool/pool.go
index 2ba62a93fd2..46a91a2563a 100644
--- a/go/pools/smartconnpool/pool.go
+++ b/go/pools/smartconnpool/pool.go
@@ -442,16 +442,19 @@ func (pool *ConnPool[C]) tryReturnConn(conn *Pooled[C]) bool {
 }
 
 func (pool *ConnPool[C]) pop(stack *connStack[C]) *Pooled[C] {
-	var conn *Pooled[C]
-	var ok bool
-
-	for conn, ok = stack.Pop(); ok; conn, ok = stack.Pop() {
+	// retry-loop: pop a connection from the stack and atomically check whether
+	// its timeout has elapsed. If the timeout has elapsed, the borrow will fail,
+	// which means that a background worker has already marked this connection
+	// as stale and is in the process of shutting it down. If we successfully mark
+	// the timeout as borrowed, we know that background workers will not be able
+	// to expire this connection (even if it's still visible to them), so it's
+	// safe to return it
+	for conn, ok := stack.Pop(); ok; conn, ok = stack.Pop() {
 		if conn.timeUsed.borrow() {
-			break
+			return conn
 		}
 	}
-
-	return conn
+	return nil
 }
 
 func (pool *ConnPool[C]) tryReturnAnyConn() bool {
@@ -748,6 +751,13 @@ func (pool *ConnPool[C]) closeIdleResources(now time.Time) {
 	mono := monotonicFromTime(now)
 
 	closeInStack := func(s *connStack[C]) {
+		// Do a read-only best effort iteration of all the connection in this
+		// stack and atomically attempt to mark them as expired.
+		// Any connections that are marked as expired are _not_ removed from
+		// the stack; it's generally unsafe to remove nodes from the stack
+		// besides the head. When clients pop from the stack, they'll immediately
+		// notice the expired connection and ignore it.
+		// see: timestamp.expired
 		for conn := s.Peek(); conn != nil; conn = conn.next.Load() {
 			if conn.timeUsed.expired(mono, timeout) {
 				pool.Metrics.idleClosed.Add(1)
diff --git a/go/pools/smartconnpool/timestamp.go b/go/pools/smartconnpool/timestamp.go
index 6654ac35269..961ff18a5c5 100644
--- a/go/pools/smartconnpool/timestamp.go
+++ b/go/pools/smartconnpool/timestamp.go
@@ -8,37 +8,62 @@ import (
 
 var monotonicRoot = time.Now()
 
+// timestamp is a monotonic point in time, stored as a number of
+// nanoseconds since the monotonic root. This type is only 8 bytes
+// and hence can always be accessed atomically
 type timestamp struct {
 	nano atomic.Int64
 }
 
+// timestampExpired is a special value that means this timestamp is now past
+// an arbitrary expiration point, and hence doesn't need to store
 const timestampExpired = math.MaxInt64
-const timestampBusy = math.MinInt64
 
-func (t *timestamp) update() {
-	t.nano.Store(int64(monotonicNow()))
-}
+// timestampBusy is a special value that means this timestamp no longer
+// tracks an expiration point
+const timestampBusy = math.MinInt64
 
+// monotonicNow returns the current monotonic time as a time.Duration.
+// This is a very efficient operation because time.Since performs direct
+// substraction of monotonic times without considering the wall clock times.
 func monotonicNow() time.Duration {
 	return time.Since(monotonicRoot)
 }
 
+// monotonicFromTime converts a wall-clock time from time.Now into a
+// monotonic timestamp.
+// This is a very efficient operation because time.(*Time).Sub performs direct
+// substraction of monotonic times without considering the wall clock times.
 func monotonicFromTime(now time.Time) time.Duration {
 	return now.Sub(monotonicRoot)
 }
 
+// set sets this timestamp to the given monotonic value
 func (t *timestamp) set(mono time.Duration) {
 	t.nano.Store(int64(mono))
 }
 
+// get returns the monotonic time of this timestamp as the number of nanoseconds
+// since the monotonic root.
 func (t *timestamp) get() time.Duration {
 	return time.Duration(t.nano.Load())
 }
 
+// elapsed returns the number of nanoseconds that have passed since
+// this timestamp was updated
 func (t *timestamp) elapsed() time.Duration {
 	return monotonicNow() - t.get()
 }
 
+// update sets this timestamp's value to the current monotonic time
+func (t *timestamp) update() {
+	t.nano.Store(int64(monotonicNow()))
+}
+
+// borrow attempts to borrow this timestamp atomically.
+// It only succeeds if we can ensure that nobody else has marked
+// this timestamp as expired. When succeeded, the timestamp
+// is cleared as "busy" as it no longer tracks an expiration point.
 func (t *timestamp) borrow() bool {
 	stamp := t.nano.Load()
 	switch stamp {
@@ -51,6 +76,9 @@ func (t *timestamp) borrow() bool {
 	}
 }
 
+// expired attempts to atomically expire this timestamp.
+// It only succeeds if we can ensure the timestamp hasn't been
+// concurrently expired or borrowed.
 func (t *timestamp) expired(now time.Duration, timeout time.Duration) bool {
 	stamp := t.nano.Load()
 	if stamp == timestampExpired {