fix(scripted-alarms): resolve Medium code-review finding (Core.ScriptedAlarms-005)
Add _disposed re-checks inside ReevaluateAsync and ShelvingCheckAsync after acquiring _evalGate so callbacks in flight when Dispose() runs bail out cleanly instead of mutating _alarms or writing to a disposed store. Drop the _alarms.Clear() from Dispose() — clearing outside the gate races concurrent reads and is unnecessary since the object is being discarded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -266,6 +266,12 @@ public sealed class ScriptedAlarmEngine : IDisposable
|
|||||||
await _evalGate.WaitAsync(ct).ConfigureAwait(false);
|
await _evalGate.WaitAsync(ct).ConfigureAwait(false);
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
|
// Re-check after acquiring the gate: a Dispose() call may have
|
||||||
|
// completed between our _evalGate.WaitAsync and here. Writing to a
|
||||||
|
// disposing store or mutating _alarms after clear is unsafe.
|
||||||
|
// (Core.ScriptedAlarms-005)
|
||||||
|
if (_disposed) return;
|
||||||
|
|
||||||
foreach (var id in alarmIds)
|
foreach (var id in alarmIds)
|
||||||
{
|
{
|
||||||
if (!_alarms.TryGetValue(id, out var state)) continue;
|
if (!_alarms.TryGetValue(id, out var state)) continue;
|
||||||
@@ -402,6 +408,13 @@ public sealed class ScriptedAlarmEngine : IDisposable
|
|||||||
await _evalGate.WaitAsync(ct).ConfigureAwait(false);
|
await _evalGate.WaitAsync(ct).ConfigureAwait(false);
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
|
// Re-check after acquiring the gate: Timer.Dispose() does not wait for
|
||||||
|
// running callbacks, so a shelving-check callback that passed the _disposed
|
||||||
|
// check in RunShelvingCheck can arrive here after Dispose() has returned.
|
||||||
|
// Mutating _alarms or saving to a disposed store here is unsafe.
|
||||||
|
// (Core.ScriptedAlarms-005)
|
||||||
|
if (_disposed) return;
|
||||||
|
|
||||||
var now = _clock();
|
var now = _clock();
|
||||||
foreach (var id in alarmIds)
|
foreach (var id in alarmIds)
|
||||||
{
|
{
|
||||||
@@ -445,7 +458,11 @@ public sealed class ScriptedAlarmEngine : IDisposable
|
|||||||
_disposed = true;
|
_disposed = true;
|
||||||
_shelvingTimer?.Dispose();
|
_shelvingTimer?.Dispose();
|
||||||
UnsubscribeFromUpstream();
|
UnsubscribeFromUpstream();
|
||||||
_alarms.Clear();
|
// Do NOT clear _alarms here: Timer.Dispose() does not wait for in-flight callbacks,
|
||||||
|
// so a ShelvingCheckAsync or ReevaluateAsync can still be running inside _evalGate.
|
||||||
|
// Those paths now re-check _disposed after acquiring the gate and bail out safely.
|
||||||
|
// Clearing _alarms outside the gate would race concurrent reads and is unnecessary
|
||||||
|
// (the whole object is being discarded). (Core.ScriptedAlarms-005)
|
||||||
_alarmsReferencing.Clear();
|
_alarmsReferencing.Clear();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user