From cd4b29a36879110c44311a4fe38540d3fd7aadb6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:01:30 -0400 Subject: [PATCH] fix(sql-browser): close two races around the registry reaper (Driver.Sql.Browser-002) (1) DisposeAsync disposed _gate unconditionally. SemaphoreSlim.Dispose does not complete pending async waiters, and the in-flight call's Release() then threw ObjectDisposedException from inside the semaphore (swallowed by the finally), so a caller parked in WaitAsync was never released and its call never returned. The gate is now left alive - the pattern SqlPollReader documents - so the waiter is admitted and meets the session's own guarded ThrowIf(_disposed, this). (2) LastUsedUtc advanced only on completion, so a slow catalog call (the AdminUI allows 20 s) was invisible to the TTL reaper for its whole duration and a session at the edge of its TTL could be disposed mid-call. It is now stamped on entry too. --- .../SqlBrowseSession.cs | 33 ++++++--- .../SqlBrowseSessionTests.cs | 69 +++++++++++++++++++ 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs index 810442bd..defef826 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs @@ -205,8 +205,18 @@ internal sealed class SqlBrowseSession : IBrowseSession } /// - /// Idempotently closes the owned connection and the gate. Errors are swallowed: the registry's reaper - /// may be racing a server-side disconnect, and a failed close must never surface in the AdminUI. + /// Idempotently closes the owned connection. Errors are swallowed: the registry's reaper may be racing + /// a server-side disconnect, and a failed close must never surface in the AdminUI. + /// The gate is deliberately NOT disposed — the same call + /// SqlPollReader makes, and for a sharper reason here. This session is disposed by the + /// registry's TTL reaper, on its own thread, while a picker may have a second call parked in + /// . Disposing the semaphore does not complete + /// its pending async waiters, and the in-flight call's Release() would then throw + /// from inside the semaphore — so the parked caller is never + /// released and its call never returns at all. Left undisposed, the waiter is admitted and meets the + /// guarded ObjectDisposedException.ThrowIf(_disposed, this) every other member raises. + /// A that was never handed out as a holds no + /// unmanaged resource, so not disposing it leaks nothing. /// /// A task that represents the asynchronous operation. public async ValueTask DisposeAsync() @@ -216,9 +226,6 @@ internal sealed class SqlBrowseSession : IBrowseSession try { await _connection.DisposeAsync().ConfigureAwait(false); } catch { /* best-effort: the connection may already be broken or closed. */ } - - try { _gate.Dispose(); } - catch { /* best-effort: a concurrent second dispose already tore it down. */ } } private async Task> ExpandSchemaAsync(string schema, CancellationToken ct) @@ -367,8 +374,12 @@ internal sealed class SqlBrowseSession : IBrowseSession /// /// Runs one catalog query under the gate and projects every row. The disposed check happens - /// inside the gate wait so a dispose racing an in-flight call cannot be missed, and - /// only advances on a call that actually completed. + /// inside the gate wait so a dispose racing an in-flight call cannot be missed. + /// is stamped on entry as well as on completion. The AdminUI + /// allows a catalog call 20 s (BrowserSessionService.PerCallTimeout) and a queued one waits + /// behind it; stamping only on completion leaves a long expand invisible to the registry's TTL reaper + /// for its whole duration, so a session that idled to the edge of the TTL and was then given one slow + /// call gets reaped mid-call — tearing the connection down under an open reader. /// private async Task> QueryAsync( string sql, @@ -376,6 +387,7 @@ internal sealed class SqlBrowseSession : IBrowseSession Func project, CancellationToken cancellationToken) { + LastUsedUtc = DateTime.UtcNow; ObjectDisposedException.ThrowIf(_disposed, this); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); try @@ -400,10 +412,9 @@ internal sealed class SqlBrowseSession : IBrowseSession } finally { - // A dispose that raced this call has already disposed the gate; releasing it then is harmless to - // ignore and must not mask the real result. - try { _gate.Release(); } - catch (ObjectDisposedException) { } + // Unconditional, and it cannot throw: DisposeAsync deliberately leaves the gate alive precisely + // so this release still reaches whoever is parked behind us. + _gate.Release(); } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs index 6b2ca8e7..6be390f9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs @@ -328,6 +328,75 @@ public sealed class SqlBrowseSessionTests () => session.ExpandAsync(SchemaNodeId, CancellationToken.None)); } + /// + /// A dispose racing an in-flight call must not strand the caller queued behind it. The registry's + /// TTL reaper disposes on its own thread while a picker may have two clicks outstanding, so this is the + /// ordinary shape, not a corner: the second call is parked in SemaphoreSlim.WaitAsync. + /// Disposing the gate does not complete its pending async waiters, and the in-flight call's + /// Release() then throws from inside the semaphore — so + /// the waiter is never released and the call never returns at all. The session must instead let it + /// through to the guarded ObjectDisposedException.ThrowIf(_disposed, this) every other member + /// raises. + /// + [Fact] + public async Task DisposeAsync_doesNotStrandACallerQueuedOnTheGate() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + var probe = new ConcurrencyProbe { Delay = TimeSpan.FromMilliseconds(400) }; + var probing = new ConcurrencyProbingDbConnection(db.OpenNewConnection(), probe); + var session = new SqlBrowseSession(probing, new SqliteDialect()); + + // First call takes the gate and holds it for the probe's delay; second parks in WaitAsync. + var inFlight = session.RootAsync(CancellationToken.None); + await Task.Delay(100, TestContext.Current.CancellationToken); + var queued = session.ExpandAsync(SchemaNodeId, CancellationToken.None); + + await session.DisposeAsync(); + + // The in-flight call's own outcome is not the assertion — it may fail on the closed connection. + try { await inFlight; } + catch (Exception ex) when (ex is ObjectDisposedException or InvalidOperationException) { } + + // A stranded caller surfaces as TimeoutException here, which reads as "expected + // ObjectDisposedException, got TimeoutException" — the honest description of the defect. + var disposed = await Should.ThrowAsync( + () => queued.WaitAsync(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken)); + disposed.ObjectName.ShouldBe( + typeof(SqlBrowseSession).FullName, + "the failure must be the session's own guarded throw, not the semaphore's"); + } + + /// + /// must advance when a call starts, not only when it + /// finishes. The AdminUI allows a catalog call 20 s; stamping only on completion leaves a slow expand + /// invisible to the registry's TTL reaper for its whole duration, so a session that idled up to the + /// edge of the TTL and was then given one long expand can be disposed mid-call — tearing the + /// connection down under an open reader. + /// + [Fact] + public async Task LastUsedUtc_advancesWhenACallSTARTS_notOnlyWhenItCompletes() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + var probe = new ConcurrencyProbe { Delay = TimeSpan.FromSeconds(2) }; + var probing = new ConcurrencyProbingDbConnection(db.OpenNewConnection(), probe); + await using var session = new SqlBrowseSession(probing, new SqliteDialect()); + + var atConstruction = session.LastUsedUtc; + // DateTime.UtcNow ticks at ~15.6 ms on Windows; without this the entry stamp could equal the + // construction stamp and the assertion would be about clock resolution rather than the stamp. + await Task.Delay(50, TestContext.Current.CancellationToken); + + var call = session.RootAsync(CancellationToken.None); + + // Observed WHILE the call is still running: the completion stamp cannot account for it. + await Task.Delay(300, TestContext.Current.CancellationToken); + call.IsCompleted.ShouldBeFalse("the probe's delay must still be holding the call open"); + session.LastUsedUtc.ShouldBeGreaterThan( + atConstruction, "the stamp must advance on entry, not only on completion"); + + await call; + } + /// /// The AdminUI bounds every call with a 20 s linked CTS /// (BrowserSessionService.PerCallTimeout) — the session invents no timeout of its own, it just