[performance] remove throttling timers (#1466)

* remove throttling timers, support setting retry-after, use retry-after in transport

* remove unused variables

* add throttling-retry-after to cmd flags

* update envparsing to include new throttling-retry-after

* update example config to include retry-after documentation

* also support retry-after formatted as date-time, ensure max backoff time

---------

Signed-off-by: kim <grufwub@gmail.com>

internal/config/config.go

@@ -133,9 +133,10 @@ type Configuration struct {
 	SyslogProtocol string `name:"syslog-protocol" usage:"Protocol to use when directing logs to syslog. Leave empty to connect to local syslog."`
 	SyslogAddress  string `name:"syslog-address" usage:"Address:port to send syslog logs to. Leave empty to connect to local syslog."`
 
-	AdvancedCookiesSamesite      string `name:"advanced-cookies-samesite" usage:"'strict' or 'lax', see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite"`
-	AdvancedRateLimitRequests    int    `name:"advanced-rate-limit-requests" usage:"Amount of HTTP requests to permit within a 5 minute window. 0 or less turns rate limiting off."`
-	AdvancedThrottlingMultiplier int    `name:"advanced-throttling-multiplier" usage:"Multiplier to use per cpu for http request throttling. 0 or less turns throttling off."`
+	AdvancedCookiesSamesite      string        `name:"advanced-cookies-samesite" usage:"'strict' or 'lax', see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite"`
+	AdvancedRateLimitRequests    int           `name:"advanced-rate-limit-requests" usage:"Amount of HTTP requests to permit within a 5 minute window. 0 or less turns rate limiting off."`
+	AdvancedThrottlingMultiplier int           `name:"advanced-throttling-multiplier" usage:"Multiplier to use per cpu for http request throttling. 0 or less turns throttling off."`
+	AdvancedThrottlingRetryAfter time.Duration `name:"advanced-throttling-retry-after" usage:"Retry-After duration response to send for throttled requests."`
 
 	// Cache configuration vars.
 	Cache CacheConfiguration `name:"cache"`

internal/config/flags.go

@@ -139,6 +139,7 @@ func (s *ConfigState) AddServerFlags(cmd *cobra.Command) {
 		cmd.Flags().String(AdvancedCookiesSamesiteFlag(), cfg.AdvancedCookiesSamesite, fieldtag("AdvancedCookiesSamesite", "usage"))
 		cmd.Flags().Int(AdvancedRateLimitRequestsFlag(), cfg.AdvancedRateLimitRequests, fieldtag("AdvancedRateLimitRequests", "usage"))
 		cmd.Flags().Int(AdvancedThrottlingMultiplierFlag(), cfg.AdvancedThrottlingMultiplier, fieldtag("AdvancedThrottlingMultiplier", "usage"))
+		cmd.Flags().Duration(AdvancedThrottlingRetryAfterFlag(), cfg.AdvancedThrottlingRetryAfter, fieldtag("AdvancedThrottlingRetryAfter", "usage"))
 	})
 }
 

internal/config/helpers.gen.go

@@ -1999,6 +1999,31 @@ func GetAdvancedThrottlingMultiplier() int { return global.GetAdvancedThrottling
 // SetAdvancedThrottlingMultiplier safely sets the value for global configuration 'AdvancedThrottlingMultiplier' field
 func SetAdvancedThrottlingMultiplier(v int) { global.SetAdvancedThrottlingMultiplier(v) }
 
+// GetAdvancedThrottlingRetryAfter safely fetches the Configuration value for state's 'AdvancedThrottlingRetryAfter' field
+func (st *ConfigState) GetAdvancedThrottlingRetryAfter() (v time.Duration) {
+	st.mutex.Lock()
+	v = st.config.AdvancedThrottlingRetryAfter
+	st.mutex.Unlock()
+	return
+}
+
+// SetAdvancedThrottlingRetryAfter safely sets the Configuration value for state's 'AdvancedThrottlingRetryAfter' field
+func (st *ConfigState) SetAdvancedThrottlingRetryAfter(v time.Duration) {
+	st.mutex.Lock()
+	defer st.mutex.Unlock()
+	st.config.AdvancedThrottlingRetryAfter = v
+	st.reloadToViper()
+}
+
+// AdvancedThrottlingRetryAfterFlag returns the flag name for the 'AdvancedThrottlingRetryAfter' field
+func AdvancedThrottlingRetryAfterFlag() string { return "advanced-throttling-retry-after" }
+
+// GetAdvancedThrottlingRetryAfter safely fetches the value for global configuration 'AdvancedThrottlingRetryAfter' field
+func GetAdvancedThrottlingRetryAfter() time.Duration { return global.GetAdvancedThrottlingRetryAfter() }
+
+// SetAdvancedThrottlingRetryAfter safely sets the value for global configuration 'AdvancedThrottlingRetryAfter' field
+func SetAdvancedThrottlingRetryAfter(v time.Duration) { global.SetAdvancedThrottlingRetryAfter(v) }
+
 // GetCacheGTSAccountMaxSize safely fetches the Configuration value for state's 'Cache.GTS.AccountMaxSize' field
 func (st *ConfigState) GetCacheGTSAccountMaxSize() (v int) {
 	st.mutex.Lock()

internal/middleware/throttling.go

@@ -29,17 +29,12 @@ package middleware
 import (
 	"net/http"
 	"runtime"
+	"strconv"
 	"time"
 
 	"github.com/gin-gonic/gin"
 )
 
-const (
-	errCapacityExceeded = "server capacity exceeded"
-	errTimedOut         = "timed out while waiting for a pending request to complete"
-	errContextCanceled  = "context canceled"
-)
-
 // token represents a request that is being processed.
 type token struct{}
 
@@ -73,11 +68,13 @@ type token struct{}
 //
 // If the multiplier is <= 0, a noop middleware will be returned instead.
 //
+// RetryAfter determines the Retry-After header value to be sent to throttled requests.
+//
 // Useful links:
 //
 //   - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After
 //   - https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/503
-func Throttle(cpuMultiplier int) gin.HandlerFunc {
+func Throttle(cpuMultiplier int, retryAfter time.Duration) gin.HandlerFunc {
 	if cpuMultiplier <= 0 {
 		// throttling is disabled, return a noop middleware
 		return func(c *gin.Context) {}
@@ -89,36 +86,24 @@ func Throttle(cpuMultiplier int) gin.HandlerFunc {
 		backlogChannelSize = limit + backlogLimit
 		tokens             = make(chan token, limit)
 		backlogTokens      = make(chan token, backlogChannelSize)
-		retryAfter         = "30" // seconds
-		backlogDuration    = 30 * time.Second
+		retryAfterStr      = strconv.FormatUint(uint64(retryAfter/time.Second), 10)
 	)
 
 	// prefill token channels
 	for i := 0; i < limit; i++ {
 		tokens <- token{}
 	}
-
 	for i := 0; i < backlogChannelSize; i++ {
 		backlogTokens <- token{}
 	}
 
-	// bail instructs the requester to return after retryAfter seconds, returns a 503,
-	// and writes the given message into the "error" field of a returned json object
-	bail := func(c *gin.Context, msg string) {
-		c.Header("Retry-After", retryAfter)
-		c.JSON(http.StatusServiceUnavailable, gin.H{"error": msg})
-		c.Abort()
-	}
-
 	return func(c *gin.Context) {
 		// inside this select, the caller tries to get a backlog token
 		select {
 		case <-c.Request.Context().Done():
 			// request context has been canceled already
-			bail(c, errContextCanceled)
+			return
 		case btok := <-backlogTokens:
-			// take a backlog token and wait
-			timer := time.NewTimer(backlogDuration)
 			defer func() {
 				// when we're finished, return the backlog token to the bucket
 				backlogTokens <- btok
@@ -127,16 +112,11 @@ func Throttle(cpuMultiplier int) gin.HandlerFunc {
 			// inside *this* select, the caller has a backlog token,
 			// and they're waiting for their turn to be processed
 			select {
-			case <-timer.C:
-				// waiting too long in the backlog
-				bail(c, errTimedOut)
 			case <-c.Request.Context().Done():
 				// the request context has been canceled already
-				timer.Stop()
-				bail(c, errContextCanceled)
+				return
 			case tok := <-tokens:
 				// the caller gets a token, so their request can now be processed
-				timer.Stop()
 				defer func() {
 					// whatever happens to the request, put the
 					// token back in the bucket when we're finished
@@ -147,7 +127,9 @@ func Throttle(cpuMultiplier int) gin.HandlerFunc {
 
 		default:
 			// we don't have space in the backlog queue
-			bail(c, errCapacityExceeded)
+			c.Header("Retry-After", retryAfterStr)
+			c.JSON(http.StatusTooManyRequests, gin.H{"error": "server capacity exceeded"})
+			c.Abort()
 		}
 	}
 }

internal/transport/transport.go

@@ -27,6 +27,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -84,36 +85,37 @@ type transport struct {
 }
 
 // GET will perform given http request using transport client, retrying on certain preset errors, or if status code is among retryOn.
-func (t *transport) GET(r *http.Request, retryOn ...int) (*http.Response, error) {
+func (t *transport) GET(r *http.Request) (*http.Response, error) {
 	if r.Method != http.MethodGet {
 		return nil, errors.New("must be GET request")
 	}
 	return t.do(r, func(r *http.Request) error {
 		return t.signGET(r)
-	}, retryOn...)
+	})
 }
 
 // POST will perform given http request using transport client, retrying on certain preset errors, or if status code is among retryOn.
-func (t *transport) POST(r *http.Request, body []byte, retryOn ...int) (*http.Response, error) {
+func (t *transport) POST(r *http.Request, body []byte) (*http.Response, error) {
 	if r.Method != http.MethodPost {
 		return nil, errors.New("must be POST request")
 	}
 	return t.do(r, func(r *http.Request) error {
 		return t.signPOST(r, body)
-	}, retryOn...)
+	})
 }
 
-func (t *transport) do(r *http.Request, signer func(*http.Request) error, retryOn ...int) (*http.Response, error) {
-	const maxRetries = 5
+func (t *transport) do(r *http.Request, signer func(*http.Request) error) (*http.Response, error) {
+	const (
+		// max no. attempts
+		maxRetries = 5
 
-	var (
-		// Initial backoff duration
-		backoff = 2 * time.Second
-
-		// Get request hostname
-		host = r.URL.Hostname()
+		// starting backoff duration.
+		baseBackoff = 2 * time.Second
 	)
 
+	// Get request hostname
+	host := r.URL.Hostname()
+
 	// Check if recently reached max retries for this host
 	// so we don't need to bother reattempting it. The only
 	// errors that are retried upon are server failure and
@@ -137,6 +139,8 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error, retryO
 	r.Header.Set("User-Agent", t.controller.userAgent)
 
 	for i := 0; i < maxRetries; i++ {
+		var backoff time.Duration
+
 		// Reset signing header fields
 		now := t.controller.clock.Now().UTC()
 		r.Header.Set("Date", now.Format("Mon, 02 Jan 2006 15:04:05")+" GMT")
@@ -152,18 +156,35 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error, retryO
 
 		// Attempt to perform request
 		rsp, err := t.controller.client.Do(r)
-		if err == nil { //nolint shutup linter
+		if err == nil { //nolint:gocritic
 			// TooManyRequest means we need to slow
 			// down and retry our request. Codes over
 			// 500 generally indicate temp. outages.
 			if code := rsp.StatusCode; code < 500 &&
-				code != http.StatusTooManyRequests &&
-				!containsInt(retryOn, rsp.StatusCode) {
+				code != http.StatusTooManyRequests {
 				return rsp, nil
 			}
 
 			// Generate error from status code for logging
 			err = errors.New(`http response "` + rsp.Status + `"`)
+
+			// Search for a provided "Retry-After" header value.
+			if after := rsp.Header.Get("Retry-After"); after != "" {
+
+				if u, _ := strconv.ParseUint(after, 10, 32); u != 0 {
+					// An integer number of backoff seconds was provided.
+					backoff = time.Duration(u) * time.Second
+				} else if at, _ := http.ParseTime(after); !at.Before(now) {
+					// An HTTP formatted future date-time was provided.
+					backoff = at.Sub(now)
+				}
+
+				// Don't let their provided backoff exceed our max.
+				if max := baseBackoff * maxRetries; backoff > max {
+					backoff = max
+				}
+			}
+
 		} else if errorsv2.Is(err,
 			context.DeadlineExceeded,
 			context.Canceled,
@@ -179,11 +200,18 @@ func (t *transport) do(r *http.Request, signer func(*http.Request) error, retryO
 		} else if errors.As(err, &x509.UnknownAuthorityError{}) {
 			// Unknown authority errors we do NOT recover from
 			return nil, err
-		} else if fastFail {
+		}
+
+		if fastFail {
 			// on fast-fail, don't bother backoff/retry
 			return nil, fmt.Errorf("%w (fast fail)", err)
 		}
 
+		if backoff == 0 {
+			// No retry-after found, set our predefined backoff.
+			backoff = time.Duration(i) * baseBackoff
+		}
+
 		l.Errorf("backing off for %s after http request error: %v", backoff.String(), err)
 
 		select {
@@ -238,13 +266,3 @@ func (t *transport) safesign(sign func()) {
 	// Perform signing
 	sign()
 }
-
-// containsInt checks if slice contains check.
-func containsInt(slice []int, check int) bool {
-	for _, i := range slice {
-		if i == check {
-			return true
-		}
-	}
-	return false
-}