Fix concerns after review

Author: Karl Solgård

src/ConcurrencyLimits.AspNetCore/ConcurrencyLimitMiddleware.cs

@@ -43,7 +43,7 @@ public async Task InvokeAsync(HttpContext context)
                 throw;
             }
         }
-        else
+        else if (!context.Response.HasStarted)
         {
             context.Response.StatusCode = _throttleStatus;
             await context.Response.WriteAsync("Concurrency limit exceeded");

src/ConcurrencyLimits.Grpc/Client/ConcurrencyLimitClientInterceptor.cs

@@ -34,7 +34,7 @@ public override TResponse BlockingUnaryCall<TRequest, TResponse>(
             listener.OnSuccess();
             return response;
         }
-        catch (RpcException ex) when (ex.StatusCode == StatusCode.Unavailable)
+        catch (RpcException ex) when (ex.StatusCode is StatusCode.Unavailable or StatusCode.DeadlineExceeded or StatusCode.Cancelled)
         {
             listener.OnDropped();
             throw;
@@ -85,7 +85,7 @@ private static async Task<TResponse> HandleResponse<TResponse>(Task<TResponse> i
             listener.OnSuccess();
             return response;
         }
-        catch (RpcException ex) when (ex.StatusCode == StatusCode.Unavailable)
+        catch (RpcException ex) when (ex.StatusCode is StatusCode.Unavailable or StatusCode.DeadlineExceeded or StatusCode.Cancelled)
         {
             listener.OnDropped();
             throw;

src/ConcurrencyLimits/Internal/SystemNanoTime.cs

@@ -7,11 +7,20 @@ namespace ConcurrencyLimits.Internal;
 /// </summary>
 public static class SystemNanoTime
 {
-    private static readonly double NanosPerTick = 1_000_000_000.0 / Stopwatch.Frequency;
+    private const long NanosPerSecond = 1_000_000_000L;
+    private static readonly long Frequency = Stopwatch.Frequency;
 
     /// <summary>
     /// Current value of a monotonic clock, in nanoseconds. Only differences between
     /// two readings are meaningful.
     /// </summary>
-    public static long Now() => (long)(Stopwatch.GetTimestamp() * NanosPerTick);
+    public static long Now()
+    {
+        // Integer math avoids the double-mantissa precision loss that a (ticks * nanosPerTick)
+        // multiply suffers once the timestamp grows large.
+        long ticks = Stopwatch.GetTimestamp();
+        long whole = ticks / Frequency * NanosPerSecond;
+        long frac = ticks % Frequency * NanosPerSecond / Frequency;
+        return whole + frac;
+    }
 }

src/ConcurrencyLimits/Limit/AbstractLimit.cs

@@ -6,6 +6,10 @@ public abstract class AbstractLimit : ILimit
     private readonly List<Action<int>> _listeners = new();
     private readonly object _sync = new();
 
+    /// <summary>Lock guarding algorithm state. Subclasses must hold this when reading mutable
+    /// state outside <see cref="Update"/> (e.g. in accessors or ToString) to avoid torn reads.</summary>
+    protected object SyncRoot => _sync;
+
     protected AbstractLimit(int initialLimit) => _limit = initialLimit;
 
     public void OnSample(long startTime, long rtt, int inflight, bool didDrop)

src/ConcurrencyLimits/Limit/Gradient2Limit.cs

@@ -164,9 +164,27 @@ protected override int Update(long startTime, long rtt, int inflight, bool didDr
         return (int)newLimit;
     }
 
-    public long GetLastRttNanos() => _lastRtt;
+    public long GetLastRttNanos()
+    {
+        lock (SyncRoot)
+        {
+            return _lastRtt;
+        }
+    }
 
-    public long GetRttNoLoadNanos() => (long)_longRtt.Get();
+    public long GetRttNoLoadNanos()
+    {
+        lock (SyncRoot)
+        {
+            return (long)_longRtt.Get();
+        }
+    }
 
-    public override string ToString() => $"Gradient2Limit [limit={(int)_estimatedLimit}]";
+    public override string ToString()
+    {
+        lock (SyncRoot)
+        {
+            return $"Gradient2Limit [limit={(int)_estimatedLimit}]";
+        }
+    }
 }

src/ConcurrencyLimits/Limit/GradientLimit.cs

@@ -208,9 +208,27 @@ protected override int Update(long startTime, long rtt, int inflight, bool didDr
         return (int)_estimatedLimit;
     }
 
-    public long GetLastRttNanos() => _lastRtt;
+    public long GetLastRttNanos()
+    {
+        lock (SyncRoot)
+        {
+            return _lastRtt;
+        }
+    }
 
-    public long GetRttNoLoadNanos() => (long)_rttNoLoadMeasurement.Get();
+    public long GetRttNoLoadNanos()
+    {
+        lock (SyncRoot)
+        {
+            return (long)_rttNoLoadMeasurement.Get();
+        }
+    }
 
-    public override string ToString() => $"GradientLimit [limit={(int)_estimatedLimit}, rtt_noload={GetRttNoLoadNanos() / 1e6} ms]";
+    public override string ToString()
+    {
+        lock (SyncRoot)
+        {
+            return $"GradientLimit [limit={(int)_estimatedLimit}, rtt_noload={(long)_rttNoLoadMeasurement.Get() / 1e6} ms]";
+        }
+    }
 }

src/ConcurrencyLimits/Limit/VegasLimit.cs

@@ -240,5 +240,11 @@ private int UpdateEstimatedLimit(long rtt, long rttNoLoad, int inflight, bool di
         return (int)newLimit;
     }
 
-    public override string ToString() => $"VegasLimit [limit={GetLimit()}, rtt_noload={_rttNoLoad / 1e6} ms]";
+    public override string ToString()
+    {
+        lock (SyncRoot)
+        {
+            return $"VegasLimit [limit={GetLimit()}, rtt_noload={_rttNoLoad / 1e6} ms]";
+        }
+    }
 }

src/ConcurrencyLimits/Limit/WindowedLimit.cs

@@ -99,7 +99,7 @@ public void OnSample(long startTime, long rtt, int inflight, bool didDrop)
             {
                 try
                 {
-                    if (endTime > _nextUpdateTime)
+                    if (endTime > Volatile.Read(ref _nextUpdateTime))
                     {
                         ISampleWindow current = Interlocked.Exchange(ref _sample, _sampleWindowFactory.NewInstance());
                         Volatile.Write(ref _nextUpdateTime,

src/ConcurrencyLimits/Limiter/AbstractLimiter.cs

@@ -36,12 +36,13 @@ protected AbstractLimiter(AbstractLimiterBuilder builder)
         _limitAlgorithm.NotifyOnChange(OnNewLimit);
         _bypassResolver = builder.BypassResolver;
 
+        string name = builder.ResolveName();
         builder.Registry.Gauge(MetricIds.LimitName, () => GetLimit());
-        _successCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, builder.Name, StatusTag, "success");
-        _droppedCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, builder.Name, StatusTag, "dropped");
-        _ignoredCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, builder.Name, StatusTag, "ignored");
-        _rejectedCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, builder.Name, StatusTag, "rejected");
-        _bypassCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, builder.Name, StatusTag, "bypassed");
+        _successCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, name, StatusTag, "success");
+        _droppedCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, name, StatusTag, "dropped");
+        _ignoredCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, name, StatusTag, "ignored");
+        _rejectedCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, name, StatusTag, "rejected");
+        _bypassCounter = builder.Registry.Counter(MetricIds.CallName, IdTag, name, StatusTag, "bypassed");
     }
 
     public abstract IListener? Acquire(TContext context);

src/ConcurrencyLimits/Limiter/AbstractLimiterBuilder.cs

@@ -11,13 +11,17 @@ public abstract class AbstractLimiterBuilder
 {
     internal ILimit Limit = VegasLimit.NewDefault();
     internal Func<long> Clock = SystemNanoTime.Now;
-    internal string Name = "unnamed-" + Interlocked.Increment(ref _idCounter);
+    internal string? Name;
     internal IMetricRegistry Registry = EmptyMetricRegistry.Instance;
 
     internal static readonly Func<object?, bool> AlwaysFalse = _ => false;
     internal Func<object?, bool> BypassResolver = AlwaysFalse;
 
     private static int _idCounter;
+
+    /// <summary>Resolve the limiter name, allocating an "unnamed-N" id lazily at build time so
+    /// ids reflect actual limiters built, not every builder instantiated.</summary>
+    internal string ResolveName() => Name ??= "unnamed-" + Interlocked.Increment(ref _idCounter);
 }
 
 public abstract class AbstractLimiterBuilder<TBuilder> : AbstractLimiterBuilder