fix(communication): evict idle SiteStreamGrpcClientFactory (site, endpoint) clients (ScadaBridge#67)

Each cached client records when it was last handed out; a periodic sweep (half of the new
CommunicationOptions.GrpcClientIdleTtl, default 30 min, 0 disables) disposes and removes entries
unused for the TTL that have no live subscription (SiteStreamGrpcClient.ActiveSubscriptionCount).
Lookup and the sweep's check-and-remove share one lock so a handed-out client is never disposed
under its caller; an evicted client is re-created on next GetOrCreate.
This commit is contained in:
Joseph Doherty
2026-11-09 12:07:28 -05:00
parent e1734675e9
commit e2b15bd520
6 changed files with 300 additions and 14 deletions
@@ -223,6 +223,16 @@ public class CommunicationOptions
/// </summary>
public TimeSpan LiveAlarmCacheLinger { get; set; } = TimeSpan.FromSeconds(30);
/// <summary>
/// How long a cached central-to-site stream client (one per <c>(site, gRPC endpoint)</c>) may sit
/// unused - no <c>GetOrCreate</c>/<c>TryGet</c> and no live subscription - before
/// <see cref="Grpc.SiteStreamGrpcClientFactory"/> disposes it and its HTTP/2 channel. Reclaims the
/// channel of an endpoint that was renamed or removed from a site's address pair; a live client
/// is simply re-created on next use (ScadaBridge#67). <see cref="TimeSpan.Zero"/> disables
/// idle eviction (clients then live until site removal or shutdown). Default 30 minutes.
/// </summary>
public TimeSpan GrpcClientIdleTtl { get; set; } = TimeSpan.FromMinutes(30);
/// <summary>
/// Cadence of the per-site aggregator's periodic reconcile snapshot fan-out. The
/// backstop that corrects instance-set drift (enable/disable/deploy/delete) and any
@@ -113,6 +113,10 @@ public sealed class CommunicationOptionsValidator : OptionsValidatorBase<Communi
+ "entries (a site node must list at least one central gRPC endpoint; central nodes "
+ "leave it empty).");
// Idle TTL of the cached site-stream gRPC clients; zero disables eviction, negative is invalid.
builder.RequireThat(options.GrpcClientIdleTtl >= TimeSpan.Zero,
$"ScadaBridge:Communication:GrpcClientIdleTtl must be zero (disabled) or a positive duration (was {options.GrpcClientIdleTtl}).");
// ── Aggregated live alarm cache ──────────────────────────────────────────
// Linger drives a Timer dueTime; TimeSpan.Zero is valid (stop the aggregator
// immediately when the last viewer leaves), only a negative value is invalid.
@@ -131,6 +131,12 @@ public class SiteStreamGrpcClient : IAsyncDisposable, IDisposable
ConnectedHeaderTimeout = options.ConnectedHeaderTimeout;
}
/// <summary>
/// Number of live subscriptions currently registered on this client. The owning
/// <see cref="SiteStreamGrpcClientFactory"/> never evicts a client while this is non-zero.
/// </summary>
public virtual int ActiveSubscriptionCount => _subscriptions.Count;
/// <summary>
/// Registers a CancellationTokenSource for a correlation ID. Test-only.
/// </summary>
@@ -16,14 +16,29 @@ namespace ZB.MOM.WW.ScadaBridge.Communication.Grpc;
/// is the only shared-disposal path — it disposes every endpoint for that site.
/// </para>
/// <para>
/// Trade-off: an edited gRPC address leaves the old endpoint's now-idle channel
/// cached until site removal or process shutdown — bounded at a handful of entries
/// per site (2 nodes + rare edits), each an idle HTTP/2 channel.
/// Idle eviction (ScadaBridge#67): an edited or removed gRPC address would otherwise leave the
/// old endpoint's channel resident until site removal or shutdown. Each entry records when it
/// was last handed out; a periodic sweep (<see cref="CommunicationOptions.GrpcClientIdleTtl"/>)
/// disposes entries that have been unused for the TTL AND have no live subscription. Lookups
/// and the sweep's check-and-remove share one lock, so a client can never be disposed after
/// being handed out; callers re-request via <see cref="GetOrCreate"/> on every (re)connect, and
/// an evicted client is simply re-created.
/// </para>
/// </summary>
public class SiteStreamGrpcClientFactory : IAsyncDisposable, IDisposable
{
private readonly ConcurrentDictionary<(string Site, string Endpoint), SiteStreamGrpcClient> _clients = new();
private sealed class Entry(SiteStreamGrpcClient client, long lastUsedTimestamp)
{
public SiteStreamGrpcClient Client { get; } = client;
public long LastUsedTimestamp { get; set; } = lastUsedTimestamp;
}
private readonly ConcurrentDictionary<(string Site, string Endpoint), Entry> _clients = new();
// Serialises hand-out (lookup + touch) against the idle sweep's check-and-remove.
private readonly object _gate = new();
private readonly TimeProvider _timeProvider;
private readonly TimeSpan _idleTtl;
private readonly ITimer? _sweepTimer;
private readonly ILoggerFactory _loggerFactory;
private readonly CommunicationOptions _options;
private readonly ISitePskProvider? _pskProvider;
@@ -64,11 +79,20 @@ public class SiteStreamGrpcClientFactory : IAsyncDisposable, IDisposable
public SiteStreamGrpcClientFactory(
ILoggerFactory loggerFactory,
IOptions<CommunicationOptions> options,
ISitePskProvider? pskProvider)
ISitePskProvider? pskProvider,
TimeProvider? timeProvider = null)
{
_loggerFactory = loggerFactory;
_options = options.Value;
_pskProvider = pskProvider;
_timeProvider = timeProvider ?? TimeProvider.System;
_idleTtl = _options.GrpcClientIdleTtl;
if (_idleTtl > TimeSpan.Zero)
{
// Sweep at half the TTL, so an entry is reclaimed within 1.5 x TTL of its last use.
var period = TimeSpan.FromTicks(Math.Max(_idleTtl.Ticks / 2, TimeSpan.FromSeconds(1).Ticks));
_sweepTimer = _timeProvider.CreateTimer(_ => EvictIdle(), null, period, period);
}
}
/// <summary>
@@ -80,8 +104,23 @@ public class SiteStreamGrpcClientFactory : IAsyncDisposable, IDisposable
/// <param name="siteIdentifier">Unique site identifier (first half of the cache key).</param>
/// <param name="grpcEndpoint">gRPC endpoint (second half of the cache key) the client is bound to.</param>
/// <returns>The cached or newly-created client bound to <paramref name="grpcEndpoint"/>.</returns>
public virtual SiteStreamGrpcClient GetOrCreate(string siteIdentifier, string grpcEndpoint) =>
_clients.GetOrAdd((siteIdentifier, grpcEndpoint), key => CreateClient(key.Site, key.Endpoint));
public virtual SiteStreamGrpcClient GetOrCreate(string siteIdentifier, string grpcEndpoint)
{
lock (_gate)
{
var key = (siteIdentifier, grpcEndpoint);
if (!_clients.TryGetValue(key, out var entry))
{
entry = new Entry(CreateClient(siteIdentifier, grpcEndpoint), _timeProvider.GetTimestamp());
_clients[key] = entry;
}
else
{
entry.LastUsedTimestamp = _timeProvider.GetTimestamp();
}
return entry.Client;
}
}
/// <summary>
/// Returns the cached client for <c>(site, endpoint)</c>, or <c>null</c> — never creates.
@@ -91,8 +130,66 @@ public class SiteStreamGrpcClientFactory : IAsyncDisposable, IDisposable
/// <param name="siteIdentifier">Unique site identifier (first half of the cache key).</param>
/// <param name="grpcEndpoint">gRPC endpoint (second half of the cache key).</param>
/// <returns>The cached client, or <c>null</c> when no client is cached for the pair.</returns>
public virtual SiteStreamGrpcClient? TryGet(string siteIdentifier, string grpcEndpoint) =>
_clients.TryGetValue((siteIdentifier, grpcEndpoint), out var client) ? client : null;
public virtual SiteStreamGrpcClient? TryGet(string siteIdentifier, string grpcEndpoint)
{
lock (_gate)
{
if (!_clients.TryGetValue((siteIdentifier, grpcEndpoint), out var entry))
return null;
entry.LastUsedTimestamp = _timeProvider.GetTimestamp();
return entry.Client;
}
}
/// <summary>
/// Disposes and removes every cached client that has been unused for at least
/// <see cref="CommunicationOptions.GrpcClientIdleTtl"/> and has no live subscription.
/// Run periodically by the factory's sweep timer; internal so tests can drive it directly.
/// Thread-safe: the check-and-remove runs under the same lock as <see cref="GetOrCreate"/>,
/// and disposal happens outside it.
/// </summary>
/// <returns>The number of clients evicted.</returns>
internal int EvictIdle()
{
if (_idleTtl <= TimeSpan.Zero)
return 0;
List<((string Site, string Endpoint) Key, SiteStreamGrpcClient Client)>? evicted = null;
lock (_gate)
{
var now = _timeProvider.GetTimestamp();
foreach (var kv in _clients)
{
if (_timeProvider.GetElapsedTime(kv.Value.LastUsedTimestamp, now) < _idleTtl)
continue;
if (kv.Value.Client.ActiveSubscriptionCount > 0)
continue; // a live stream is using the channel - it is not idle
if (_clients.TryRemove(kv.Key, out var removed))
(evicted ??= []).Add((kv.Key, removed.Client));
}
}
if (evicted is null)
return 0;
foreach (var (key, client) in evicted)
{
_loggerFactory.CreateLogger<SiteStreamGrpcClientFactory>().LogInformation(
"Evicting idle site stream gRPC client for site {SiteId} endpoint {Endpoint}",
key.Site, key.Endpoint);
// Synchronous dispose: the client releases everything synchronously, and the
// timer callback must not block on or leak an async path. Fault-isolated.
try { client.Dispose(); }
catch (Exception ex)
{
_loggerFactory.CreateLogger<SiteStreamGrpcClientFactory>().LogWarning(
ex, "Disposing the idle site stream gRPC client for site {SiteId} endpoint {Endpoint} failed",
key.Site, key.Endpoint);
}
}
return evicted.Count;
}
/// <summary>
/// Creates a single <see cref="SiteStreamGrpcClient"/>. Overridable so tests
@@ -119,8 +216,13 @@ public class SiteStreamGrpcClientFactory : IAsyncDisposable, IDisposable
{
foreach (var key in _clients.Keys.Where(k => k.Site == siteIdentifier).ToList())
{
if (_clients.TryRemove(key, out var client))
await DisposeClientAsync(client, key);
Entry? entry;
lock (_gate)
{
_clients.TryRemove(key, out entry);
}
if (entry is not null)
await DisposeClientAsync(entry.Client, key);
}
// Drop the cached preshared key too, so a site removed and re-added under the same
@@ -136,9 +238,10 @@ public class SiteStreamGrpcClientFactory : IAsyncDisposable, IDisposable
/// <returns>A value task that completes when all clients have been disposed.</returns>
public async ValueTask DisposeAsync()
{
_sweepTimer?.Dispose();
foreach (var entry in _clients)
{
await DisposeClientAsync(entry.Value, entry.Key);
await DisposeClientAsync(entry.Value.Client, entry.Key);
}
_clients.Clear();
}
@@ -174,11 +277,12 @@ public class SiteStreamGrpcClientFactory : IAsyncDisposable, IDisposable
/// </summary>
public void Dispose()
{
_sweepTimer?.Dispose();
foreach (var entry in _clients)
{
try
{
entry.Value.Dispose();
entry.Value.Client.Dispose();
}
catch (Exception ex)
{