Skip to content

Commit 4228ad1

Browse files
committed
fix(http): evict stale devices and isolate DeviceReceived handler faults
Lands four §19 review findings on the issue-176 surface: - F-001: clear the _devices cache at the start of every probe inside the same lock block that clears _cachedComponents and _cachedDataItems so devices the agent no longer advertises are evicted before the new probe is loaded. The Devices accessor's XML doc claimed the snapshot reflected the most recent probe; without the clear, stale UUIDs persisted indefinitely. - F-002: wrap DeviceReceived?.Invoke in try/catch and route subscriber exceptions through InternalError. A throwing handler previously aborted the populate loop mid-stream, leaving the cache half-filled and suppressing ProbeReceived for that probe. The Worker loop's outer catch around the same fan-out at line 860 already used this pattern; this finding extends it to the per-device event. - F-004: wrap the snapshot in ReadOnlyDictionary so a consumer cannot mutate the cache through a downcast. The Devices XML doc promised a read-only contract; without the wrapper the runtime type stayed a plain Dictionary that downcasts could mutate. - F-005: tighten spaced em-dashes in the DeviceReceived summary to the unspaced form per CONVENTIONS §1.0d-decies. Adds three NUnit tests pinning the new invariants and closing two §10a negative-coverage gaps the review surfaced (F-008, F-009): - DeviceReceivedFiresOnEverySubsequentProbe — restarts the client so a second probe round-trip runs through the same agent and asserts the event re-fires for every device on the second probe. - DevicesAccessorReflectsLatestProbeWithStaleEntriesEvicted — pins the F-001 eviction contract. - DeviceReceivedHandlerThrowingDoesNotBreakCachePopulation — pins the F-002 isolation contract.
1 parent c5ff7b7 commit 4228ad1

2 files changed

Lines changed: 171 additions & 12 deletions

File tree

libraries/MTConnect.NET-HTTP/Clients/MTConnectHttpClient.cs

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
using MTConnect.Streams;
1212
using System;
1313
using System.Collections.Generic;
14+
using System.Collections.ObjectModel;
1415
using System.Linq;
1516
using System.Threading;
1617
using System.Threading.Tasks;
@@ -183,7 +184,9 @@ private void Init()
183184
/// A snapshot of the device model received from the most recent Probe response,
184185
/// keyed by device UUID. The dictionary is empty until the first probe succeeds.
185186
/// Each call returns a fresh snapshot reflecting the most recently completed probe;
186-
/// previously returned snapshots are unaffected by subsequent probes. Each
187+
/// previously returned snapshots are unaffected by subsequent probes, and devices
188+
/// that disappear between probes are evicted at the start of the next probe so the
189+
/// snapshot never carries entries the agent no longer advertises. Each
187190
/// <see cref="IDevice"/>'s DataItems carry the fully wired
188191
/// <see cref="IDataItem.Container"/> and <see cref="IDataItem.Device"/>
189192
/// back-pointers set during parsing, so consumers can walk the component ancestry
@@ -192,23 +195,30 @@ private void Init()
192195
/// <remarks>
193196
/// <para>
194197
/// The dictionary returned by each call is a fresh allocation independent of the
195-
/// cache; the <see cref="IDevice"/> values within are shared references to cached
196-
/// instances that the client replaces wholesale on each probe. The accessor
197-
/// acquires the client's internal lock, so callers may enumerate the snapshot
198-
/// without synchronizing against the worker thread that processes subsequent probes.
198+
/// cache, wrapped in a <see cref="ReadOnlyDictionary{TKey, TValue}"/>
199+
/// so consumers cannot mutate the snapshot through a downcast. The <see cref="IDevice"/>
200+
/// values within are shared references to cached instances that the client replaces
201+
/// wholesale on each probe. The accessor acquires the client's internal lock, so
202+
/// callers may enumerate the snapshot without synchronizing against the worker thread
203+
/// that processes subsequent probes.
199204
/// </para>
200205
/// <para>
201206
/// Each access allocates a fresh dictionary. Cache the returned reference if you
202207
/// read repeatedly between probes.
203208
/// </para>
209+
/// <para>
210+
/// Empty probe responses (where the agent answers with no devices) do not clear the
211+
/// cache; the previous snapshot stays presented until the next non-empty probe.
212+
/// </para>
204213
/// </remarks>
205214
public IReadOnlyDictionary<string, IDevice> Devices
206215
{
207216
get
208217
{
209218
lock (_lock)
210219
{
211-
return new Dictionary<string, IDevice>(_devices);
220+
return new ReadOnlyDictionary<string, IDevice>(
221+
new Dictionary<string, IDevice>(_devices));
212222
}
213223
}
214224
}
@@ -218,15 +228,17 @@ public IReadOnlyDictionary<string, IDevice> Devices
218228

219229
/// <summary>
220230
/// Raised once per parsed device for every Probe response the client receives,
221-
/// carrying the fully wired <see cref="IDevice"/> instancethe same instance
222-
/// the <see cref="Devices"/> snapshot accessor would return for that UUIDwith
223-
/// its DataItems' <see cref="IDataItem.Container"/> and <see cref="IDataItem.Device"/>
231+
/// carrying the fully wired <see cref="IDevice"/> instancethe same instance the
232+
/// <see cref="Devices"/> snapshot accessor would return for that UUIDwith its
233+
/// DataItems' <see cref="IDataItem.Container"/> and <see cref="IDataItem.Device"/>
224234
/// back-pointers set, and the agent's <c>InstanceId</c> stamped on each DataItem.
225235
/// </summary>
226236
/// <remarks>
227237
/// Handlers fire in document order while the cache is still being populated.
228238
/// Subscribe to <see cref="ProbeReceived"/> to receive notification after the full
229-
/// probe response has been processed.
239+
/// probe response has been processed. A handler that throws is isolated by the
240+
/// client: the exception is forwarded through <see cref="InternalError"/> and the
241+
/// cache fill plus subsequent fan-out continue normally.
230242
/// </remarks>
231243
public event EventHandler<IDevice> DeviceReceived;
232244

@@ -871,11 +883,13 @@ private void ProcessProbeDocument(IDevicesResponseDocument document)
871883
{
872884
if (document != null && !document.Devices.IsNullOrEmpty())
873885
{
874-
// Clear Cached DataItems and Components
886+
// Clear cached DataItems, Components, and the device snapshot so devices
887+
// the agent no longer advertises are evicted before the new probe is loaded.
875888
lock (_lock)
876889
{
877890
_cachedComponents.Clear();
878891
_cachedDataItems.Clear();
892+
_devices.Clear();
879893
}
880894

881895
foreach (var device in document.Devices)
@@ -890,7 +904,16 @@ private void ProcessProbeDocument(IDevicesResponseDocument document)
890904
}
891905

892906
// Fire per-device inside the populate loop so the cache and event stay in lockstep.
893-
DeviceReceived?.Invoke(this, outputDevice);
907+
// Isolate subscriber exceptions so one bad handler cannot abort the populate loop
908+
// or suppress ProbeReceived; route the fault through InternalError instead.
909+
try
910+
{
911+
DeviceReceived?.Invoke(this, outputDevice);
912+
}
913+
catch (Exception ex)
914+
{
915+
InternalError?.Invoke(this, ex);
916+
}
894917
}
895918

896919
// Raise ProbeReceived Event

tests/MTConnect.NET-HTTP-Tests/Clients/HttpClientDeviceModel.cs

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,5 +190,141 @@ public void DeviceReceivedCarriesWiredDataItemBackPointers()
190190
client.Stop();
191191
}
192192
}
193+
194+
/// <summary>Pins the behaviour expressed by the test name: device received fires on every subsequent probe, not just the first.</summary>
195+
[Test]
196+
public void DeviceReceivedFiresOnEverySubsequentProbe()
197+
{
198+
var client = new MTConnectHttpClient(Hostname, Port);
199+
200+
using var firstProbeSignal = new ManualResetEventSlim(false);
201+
using var secondProbeSignal = new ManualResetEventSlim(false);
202+
var probeCount = 0;
203+
client.ProbeReceived += (_, _) =>
204+
{
205+
var n = Interlocked.Increment(ref probeCount);
206+
if (n == 1) firstProbeSignal.Set();
207+
else if (n == 2) secondProbeSignal.Set();
208+
};
209+
210+
var deviceFireCount = 0;
211+
client.DeviceReceived += (_, _) => Interlocked.Increment(ref deviceFireCount);
212+
213+
try
214+
{
215+
client.Start();
216+
Assert.That(firstProbeSignal.Wait(ProbeWaitTimeoutMs), Is.True, "First ProbeReceived did not fire within the timeout");
217+
218+
var firstFireCount = Volatile.Read(ref deviceFireCount);
219+
Assert.That(firstFireCount, Is.EqualTo(ExpectedDocumentEntryCount), "DeviceReceived did not fire once per device on the first probe");
220+
}
221+
finally
222+
{
223+
client.Stop();
224+
}
225+
226+
// Restart the client to force a second probe round-trip through the same
227+
// agent; the event must fire again for every device in the second probe,
228+
// not stay quiet.
229+
try
230+
{
231+
client.Start();
232+
Assert.That(secondProbeSignal.Wait(ProbeWaitTimeoutMs), Is.True, "Second ProbeReceived did not fire within the timeout");
233+
234+
var totalFireCount = Volatile.Read(ref deviceFireCount);
235+
Assert.That(totalFireCount, Is.EqualTo(ExpectedDocumentEntryCount * 2), "DeviceReceived did not re-fire on the second probe");
236+
}
237+
finally
238+
{
239+
client.Stop();
240+
}
241+
}
242+
243+
/// <summary>Pins the behaviour expressed by the test name: devices accessor reflects the latest probe with stale entries evicted.</summary>
244+
[Test]
245+
public void DevicesAccessorReflectsLatestProbeWithStaleEntriesEvicted()
246+
{
247+
var client = new MTConnectHttpClient(Hostname, Port);
248+
249+
using var firstProbeSignal = new ManualResetEventSlim(false);
250+
using var secondProbeSignal = new ManualResetEventSlim(false);
251+
var probeCount = 0;
252+
client.ProbeReceived += (_, _) =>
253+
{
254+
var n = Interlocked.Increment(ref probeCount);
255+
if (n == 1) firstProbeSignal.Set();
256+
else if (n == 2) secondProbeSignal.Set();
257+
};
258+
259+
try
260+
{
261+
client.Start();
262+
Assert.That(firstProbeSignal.Wait(ProbeWaitTimeoutMs), Is.True, "First ProbeReceived did not fire within the timeout");
263+
}
264+
finally
265+
{
266+
client.Stop();
267+
}
268+
269+
try
270+
{
271+
client.Start();
272+
Assert.That(secondProbeSignal.Wait(ProbeWaitTimeoutMs), Is.True, "Second ProbeReceived did not fire within the timeout");
273+
274+
// The cache is cleared at the start of every probe, so the post-second-probe
275+
// snapshot count must equal the agent's device count, not double it. This
276+
// pins the eviction-on-reprobe contract surfaced by the §19 review.
277+
var snapshot = client.Devices;
278+
Assert.That(snapshot.Count, Is.EqualTo(ExpectedDocumentEntryCount), "Devices accessor accumulated devices across probes instead of evicting stale entries");
279+
}
280+
finally
281+
{
282+
client.Stop();
283+
}
284+
}
285+
286+
/// <summary>Pins the behaviour expressed by the test name: device received handler throwing does not break cache population.</summary>
287+
[Test]
288+
public void DeviceReceivedHandlerThrowingDoesNotBreakCachePopulation()
289+
{
290+
var client = new MTConnectHttpClient(Hostname, Port);
291+
292+
var internalErrorCount = 0;
293+
client.InternalError += (_, _) => Interlocked.Increment(ref internalErrorCount);
294+
295+
// First subscriber throws on every fan-out; second subscriber records every
296+
// device it sees. With exception-isolation in place, the cache must still
297+
// fully populate, the second subscriber must still see every device, and the
298+
// throw must surface through InternalError.
299+
client.DeviceReceived += (_, _) => throw new InvalidOperationException("intentional test fault");
300+
301+
var receivedDevices = new List<IDevice>();
302+
var receivedLock = new object();
303+
client.DeviceReceived += (_, device) =>
304+
{
305+
lock (receivedLock) { receivedDevices.Add(device); }
306+
};
307+
308+
using var probeSignal = new ManualResetEventSlim(false);
309+
client.ProbeReceived += (_, _) => probeSignal.Set();
310+
311+
try
312+
{
313+
client.Start();
314+
315+
Assert.That(probeSignal.Wait(ProbeWaitTimeoutMs), Is.True, "ProbeReceived did not fire within the timeout");
316+
317+
List<IDevice> snapshot;
318+
lock (receivedLock) { snapshot = new List<IDevice>(receivedDevices); }
319+
320+
Assert.That(snapshot.Count, Is.EqualTo(ExpectedDocumentEntryCount), "Throwing handler suppressed downstream subscribers");
321+
Assert.That(client.Devices.Count, Is.EqualTo(ExpectedDocumentEntryCount), "Throwing handler corrupted the cache fill");
322+
Assert.That(Volatile.Read(ref internalErrorCount), Is.GreaterThanOrEqualTo(ExpectedDocumentEntryCount), "Throwing handler did not surface through InternalError");
323+
}
324+
finally
325+
{
326+
client.Stop();
327+
}
328+
}
193329
}
194330
}

0 commit comments

Comments
 (0)