fix(ab-transport): trim-only CIP path canonicalisation, observe abandoned shared opens, no Forward Open after dispose during connect

This commit is contained in:
Joseph Doherty
2026-10-07 16:49:19 -04:00
parent a251954bf7
commit 33dbd342e5
5 changed files with 41 additions and 8 deletions
@@ -123,7 +123,14 @@ public sealed class CipConnection : ICipChannel, IAsyncDisposable
}
pending = _pendingOpen;
}
if (started is not null) _ = RunOpenAsync(started);
if (started is not null)
{
// Every waiter may have abandoned the open (its own token); observe a failure so it never
// surfaces as UnobservedTaskException.
_ = started.Task.ContinueWith(static t => _ = t.Exception, CancellationToken.None,
TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default);
_ = RunOpenAsync(started);
}
return pending.WaitAsync(ct);
}
@@ -170,6 +177,12 @@ public sealed class CipConnection : ICipChannel, IAsyncDisposable
{
ThrowIfDisposed(); // disposal may have begun while this attempt waited out the throttle
var client = await EncapsulationClient.ConnectAsync(_key.Host, _key.Port, _o.Timeout, ct).ConfigureAwait(false);
if (Volatile.Read(ref _disposed) != 0)
{
// Disposal began during the TCP connect / RegisterSession: never put a Forward Open on the wire.
await client.DisposeAsync().ConfigureAwait(false);
throw new AbTransportException(AbStatus.Abort, "Connection disposed.");
}
if (_o.Routing != CipRouting.Connected) return new Open(client, null, 0, UnconnectedMessageLimit);
try
{
@@ -17,6 +17,10 @@ public static class CipPath
{
var port = int.Parse(parts[i], CultureInfo.InvariantCulture);
var link = parts[i + 1];
// A link is a slot byte or an IP address/hostname; neither contains whitespace. Without this a
// typo like "1,2 3" would be sent as an ASCII link "2 3" instead of failing here.
if (link.Length == 0 || link.Any(char.IsWhiteSpace))
throw new ArgumentException($"CIP path '{cipPath}' has an empty or malformed link '{link}'.", nameof(cipPath));
var isByte = byte.TryParse(link, NumberStyles.None, CultureInfo.InvariantCulture, out var linkByte);
if (port < 15)
{
@@ -3,14 +3,16 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Cip;
/// <summary>
/// Identity of one physical CIP connection: exactly one <see cref="CipConnection"/> (and one Forward Open
/// throttle) exists per key (press safety). <see cref="Host"/> and <see cref="PlcAttribute"/> compare
/// case-insensitively; <see cref="Port"/> compares exactly; <see cref="CipPath"/> is canonicalised (all
/// whitespace removed, so <c>"1,0"</c> and <c>" 1, 0 "</c> are the same device) and then compares exactly.
/// case-insensitively; <see cref="Port"/> compares exactly; <see cref="CipPath"/> is canonicalised (each
/// comma-separated part trimmed, so <c>"1,0"</c> and <c>" 1, 0 "</c> are the same device) and then
/// compares exactly. Whitespace INSIDE a part is kept, so a typo such as <c>"1,2 3"</c> still fails loudly in
/// <see cref="Cip.CipPath.EncodeRoute"/> instead of silently becoming slot 23.
/// </summary>
public sealed record DeviceKey(string Host, int Port, string CipPath, string PlcAttribute)
{
private readonly string _cipPath = Canonicalise(CipPath);
/// <summary>The route, whitespace-free. Canonicalised on construction and on <c>with</c>.</summary>
/// <summary>The route with each part trimmed. Canonicalised on construction and on <c>with</c>.</summary>
public string CipPath
{
get => _cipPath;
@@ -18,7 +20,7 @@ public sealed record DeviceKey(string Host, int Port, string CipPath, string Plc
}
private static string Canonicalise(string? path) =>
path is null ? "" : string.Concat(path.Where(c => !char.IsWhiteSpace(c)));
string.IsNullOrWhiteSpace(path) ? "" : string.Join(',', path.Split(',').Select(p => p.Trim()));
public bool Equals(DeviceKey? other) =>
other is not null
@@ -24,9 +24,12 @@ public sealed class ForwardOpenThrottle
_time = time ?? throw new ArgumentNullException(nameof(time));
}
/// <summary>Run one open attempt once the throttle allows it. Throws
/// <see cref="AbTransportException"/>(<see cref="AbStatus.BadConnection"/>) when <paramref name="ct"/>
/// fires before the attempt could start; the attempt itself sees <paramref name="ct"/> too.</summary>
/// <summary>Run one open attempt once the throttle allows it. <paramref name="ct"/> bounds only the wait
/// BEFORE the attempt (queued behind an in-flight attempt, or the interval): when it fires there, this
/// throws <see cref="AbTransportException"/>(<see cref="AbStatus.BadConnection"/>, "Forward Open
/// throttled"). <paramref name="ct"/> is also passed to <paramref name="open"/>, which may ignore it:
/// <see cref="CipConnection"/> does, running the attempt under its own open deadline so that an accepted
/// Forward Open is never abandoned without a Forward Close.</summary>
public async Task<T> RunAsync<T>(Func<CancellationToken, Task<T>> open, CancellationToken ct)
{
ArgumentNullException.ThrowIfNull(open);
@@ -162,6 +162,7 @@ public sealed class DeviceSessionPoolTests
b.ShouldBe(a);
b.GetHashCode().ShouldBe(a.GetHashCode());
(a with { CipPath = "1, 2" }).CipPath.ShouldBe("1,2");
new DeviceKey("plc1", 44818, " ", "ControlLogix").CipPath.ShouldBe("");
var pool = new DeviceSessionPool(new ManualTimeProvider());
await using var la = pool.Acquire(a, Options());
@@ -170,6 +171,16 @@ public sealed class DeviceSessionPoolTests
pool.EntryCount.ShouldBe(1);
}
[Fact]
public void Whitespace_inside_a_cip_path_part_is_kept_so_a_typo_fails_loudly()
{
var typo = new DeviceKey("plc1", 44818, "1,2 3", "ControlLogix");
typo.CipPath.ShouldNotBe("1,23"); // never silently slot 23 on another controller
typo.CipPath.ShouldBe("1,2 3");
Should.Throw<ArgumentException>(() => CipPath.EncodeRoute(typo.CipPath));
CipPath.EncodeRoute(new DeviceKey("plc1", 44818, " 1, 0 ", "ControlLogix").CipPath).ShouldBe(new byte[] { 0x01, 0x00 });
}
[Fact]
public async Task A_new_connection_waits_for_every_earlier_close_not_just_the_latest()
{