fix(mqtt): make connect idempotent, bound the reconnect callback, stop State lying
Closes the connect-vs-connect defect: MQTTnet throws (and raises no DisconnectedAsync) on a connect-while-connected, so a caller's ConnectAsync and the reconnect supervisor corrupted each other in both directions. Both paths now check first; when the supervisor finds the session already restored it stands down WITHOUT firing Reconnected. Reconnected becomes Func<CancellationToken, Task>, fed from the lifetime token and capped at ConnectTimeoutSeconds, so a hung re-subscribe can no longer park the supervisor. Connected is published only after the re-subscribe succeeds; a supervisor that dies now reports Faulted instead of Reconnecting forever. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
Joseph Doherty
@@ -4,6 +4,7 @@ using System.Net.Security;
|
||||
using System.Net.Sockets;
|
||||
using System.Security.Cryptography;
|
||||
using System.Security.Cryptography.X509Certificates;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using MQTTnet;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
@@ -447,10 +448,10 @@ public sealed class MqttConnectionTests
|
||||
await using var conn = new MqttConnection(LoopbackOpts(), driverId: "t", logger: null);
|
||||
var first = 0;
|
||||
var second = 0;
|
||||
conn.Reconnected += () => { Interlocked.Increment(ref first); return Task.CompletedTask; };
|
||||
conn.Reconnected += () => { Interlocked.Increment(ref second); return Task.CompletedTask; };
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref first); return Task.CompletedTask; };
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref second); return Task.CompletedTask; };
|
||||
|
||||
await conn.FireReconnectedAsync();
|
||||
await conn.FireReconnectedAsync(TestContext.Current.CancellationToken);
|
||||
|
||||
first.ShouldBe(1);
|
||||
second.ShouldBe(1);
|
||||
@@ -467,10 +468,10 @@ public sealed class MqttConnectionTests
|
||||
{
|
||||
await using var conn = new MqttConnection(LoopbackOpts(), driverId: "t", logger: null);
|
||||
var behindTheThrower = 0;
|
||||
conn.Reconnected += () => throw new InvalidOperationException("SUBACK refused");
|
||||
conn.Reconnected += () => { Interlocked.Increment(ref behindTheThrower); return Task.CompletedTask; };
|
||||
conn.Reconnected += _ => throw new InvalidOperationException("SUBACK refused");
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref behindTheThrower); return Task.CompletedTask; };
|
||||
|
||||
await Should.ThrowAsync<Exception>(async () => await conn.FireReconnectedAsync());
|
||||
await Should.ThrowAsync<Exception>(async () => await conn.FireReconnectedAsync(TestContext.Current.CancellationToken));
|
||||
|
||||
behindTheThrower.ShouldBe(1);
|
||||
}
|
||||
@@ -480,7 +481,7 @@ public sealed class MqttConnectionTests
|
||||
{
|
||||
await using var conn = new MqttConnection(LoopbackOpts(), driverId: "t", logger: null);
|
||||
|
||||
await Should.NotThrowAsync(async () => await conn.FireReconnectedAsync());
|
||||
await Should.NotThrowAsync(async () => await conn.FireReconnectedAsync(TestContext.Current.CancellationToken));
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------------
|
||||
@@ -594,7 +595,7 @@ public sealed class MqttConnectionTests
|
||||
using var broker = new MiniBroker();
|
||||
await using var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null);
|
||||
var resubscribes = 0;
|
||||
conn.Reconnected += () => { Interlocked.Increment(ref resubscribes); return Task.CompletedTask; };
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref resubscribes); return Task.CompletedTask; };
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
|
||||
@@ -684,20 +685,248 @@ public sealed class MqttConnectionTests
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null);
|
||||
|
||||
// No assertions inside the hook: it runs inside the production try, so a Shouldly throw would
|
||||
// be laundered through Classify and resurface as a confusing TimeoutException. Record, assert
|
||||
// outside.
|
||||
var raceReproduced = false;
|
||||
conn.AfterConnectHookForTests = async () =>
|
||||
{
|
||||
_ = Task.Run(async () => await conn.DisposeAsync());
|
||||
(await WaitUntilAsync(() => conn.State == MqttConnectionState.Disposed, TimeSpan.FromSeconds(5)))
|
||||
.ShouldBeTrue("dispose never flagged the instance — the race was not reproduced");
|
||||
raceReproduced = await WaitUntilAsync(
|
||||
() => conn.State == MqttConnectionState.Disposed,
|
||||
TimeSpan.FromSeconds(5));
|
||||
};
|
||||
|
||||
await Should.ThrowAsync<ObjectDisposedException>(async () => await conn.ConnectAsync(CancellationToken.None));
|
||||
|
||||
raceReproduced.ShouldBeTrue("dispose never flagged the instance — the race was not reproduced");
|
||||
(await WaitUntilAsync(() => broker.LiveConnections == 0, TimeSpan.FromSeconds(15)))
|
||||
.ShouldBeTrue($"a live broker connection leaked ({broker.LiveConnections} still open)");
|
||||
conn.IsConnected.ShouldBeFalse();
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------------
|
||||
// C1 — connect-vs-connect: the supervisor and ConnectAsync must not corrupt each other
|
||||
// ---------------------------------------------------------------------------------
|
||||
|
||||
/// <summary>
|
||||
/// Direction (b), the simplest form: MQTTnet throws
|
||||
/// <c>InvalidOperationException: It is not allowed to connect with a server after the
|
||||
/// connection is established</c> for a connect-while-connected. That is outside this type's
|
||||
/// documented contract, so a caller re-running <c>InitializeAsync</c> against a healthy
|
||||
/// session would look like a hard driver failure.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task ConnectAsync_OnAnAlreadyEstablishedSession_IsANoOp_NotAnInvalidOperation()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
await using var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null);
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
await Should.NotThrowAsync(async () => await conn.ConnectAsync(CancellationToken.None));
|
||||
|
||||
conn.IsConnected.ShouldBeTrue();
|
||||
conn.State.ShouldBe(MqttConnectionState.Connected);
|
||||
broker.AcceptedCount.ShouldBe(1, "the second connect opened a second socket instead of no-opping");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Direction (b) through the supervisor: the supervisor restores the session, then the driver
|
||||
/// host re-runs <c>InitializeAsync</c> — exactly the sequence this design relies on.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task ConnectAsync_AfterTheSupervisorAlreadyReconnected_IsANoOp()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
await using var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null);
|
||||
var resubscribes = 0;
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref resubscribes); return Task.CompletedTask; };
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
broker.DropAll();
|
||||
(await WaitUntilAsync(
|
||||
() => Volatile.Read(ref resubscribes) >= 1 && conn.State == MqttConnectionState.Connected,
|
||||
TimeSpan.FromSeconds(30)))
|
||||
.ShouldBeTrue();
|
||||
|
||||
await Should.NotThrowAsync(async () => await conn.ConnectAsync(CancellationToken.None));
|
||||
|
||||
conn.IsConnected.ShouldBeTrue();
|
||||
conn.State.ShouldBe(MqttConnectionState.Connected);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Direction (a): a caller reconnects while the supervisor is asleep in backoff. The
|
||||
/// supervisor must find the restored session and stand down — <b>silently</b>, without firing
|
||||
/// <c>Reconnected</c> (the caller owns its own re-subscribe). Left unfixed it fails forever
|
||||
/// against a perfectly healthy connection, at Debug level, inflating <c>attempt</c> so the
|
||||
/// NEXT genuine outage waits max backoff instead of min.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task CallerReconnectsWhileTheSupervisorIsInBackoff_SupervisorStandsDownSilently()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
var log = new CapturingLogger();
|
||||
var opts = BrokerOpts(broker.Port) with { ReconnectMinBackoffSeconds = 3, ReconnectMaxBackoffSeconds = 3 };
|
||||
await using var conn = new MqttConnection(opts, driverId: "t", logger: log);
|
||||
var resubscribes = 0;
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref resubscribes); return Task.CompletedTask; };
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
broker.DropAll();
|
||||
(await WaitUntilAsync(() => conn.State == MqttConnectionState.Reconnecting, TimeSpan.FromSeconds(10)))
|
||||
.ShouldBeTrue();
|
||||
|
||||
// The caller wins the race while the supervisor sleeps off its 3 s backoff.
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
conn.IsConnected.ShouldBeTrue();
|
||||
var failuresBefore = log.CountContaining("reconnect attempt");
|
||||
|
||||
// Three more supervisor wake points would land in this window if it were still looping.
|
||||
await Task.Delay(TimeSpan.FromSeconds(9), TestContext.Current.CancellationToken);
|
||||
|
||||
log.CountContaining("reconnect attempt")
|
||||
.ShouldBe(failuresBefore, "the supervisor kept failing against a healthy connection");
|
||||
conn.State.ShouldBe(MqttConnectionState.Connected);
|
||||
conn.IsConnected.ShouldBeTrue();
|
||||
Volatile.Read(ref resubscribes).ShouldBe(0, "the caller owns its own re-subscribe; Reconnected must not fire");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The operational consequence of direction (a), measured rather than argued: after a caller
|
||||
/// has won a reconnect race, the supervisor must be parked at its outer wait with its attempt
|
||||
/// counter reset, so the NEXT genuine outage recovers at MIN backoff.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The timings are load-bearing, not arbitrary. A supervisor still flailing against the healthy
|
||||
/// connection would have burned attempts 0 and 1 (at t≈2 s and t≈6 s) and be asleep in an 8 s
|
||||
/// backoff running to t≈14 s, so the outage induced at t≈7 s cannot be answered before then.
|
||||
/// A stood-down supervisor answers it one min-backoff (2 s) later. The 5 s window separates the
|
||||
/// two by a clear margin; widening the burn only widens the separation.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AfterACallerWinsTheRace_TheNextGenuineOutageStillRecoversAtMinBackoff()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
var opts = BrokerOpts(broker.Port) with { ReconnectMinBackoffSeconds = 2, ReconnectMaxBackoffSeconds = 30 };
|
||||
await using var conn = new MqttConnection(opts, driverId: "t", logger: null);
|
||||
var resubscribes = 0;
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref resubscribes); return Task.CompletedTask; };
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
broker.DropAll();
|
||||
(await WaitUntilAsync(() => conn.State == MqttConnectionState.Reconnecting, TimeSpan.FromSeconds(10)))
|
||||
.ShouldBeTrue();
|
||||
await conn.ConnectAsync(CancellationToken.None); // caller wins the race
|
||||
await Task.Delay(TimeSpan.FromSeconds(7), TestContext.Current.CancellationToken); // a broken loop climbs here
|
||||
|
||||
broker.DropAll(); // the NEXT genuine outage
|
||||
var recovered = await WaitUntilAsync(() => Volatile.Read(ref resubscribes) >= 1, TimeSpan.FromSeconds(5));
|
||||
|
||||
recovered.ShouldBeTrue("recovery took far longer than min backoff — the attempt counter never reset");
|
||||
conn.State.ShouldBe(MqttConnectionState.Connected);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------------
|
||||
// I2 / M3 — the Reconnected callback is bounded, and Connected means "connected AND subscribed"
|
||||
// ---------------------------------------------------------------------------------
|
||||
|
||||
[Fact]
|
||||
public async Task FireReconnected_PassesTheSuppliedTokenToEverySubscriber()
|
||||
{
|
||||
await using var conn = new MqttConnection(LoopbackOpts(), driverId: "t", logger: null);
|
||||
using var cts = new CancellationTokenSource();
|
||||
var seen = new List<CancellationToken>();
|
||||
conn.Reconnected += ct => { seen.Add(ct); return Task.CompletedTask; };
|
||||
conn.Reconnected += ct => { seen.Add(ct); return Task.CompletedTask; };
|
||||
|
||||
await conn.FireReconnectedAsync(cts.Token);
|
||||
|
||||
seen.Count.ShouldBe(2);
|
||||
seen.ShouldAllBe(t => t == cts.Token);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A re-subscribe that hangs — broker accepts SUBSCRIBE and never SUBACKs, the frozen-peer
|
||||
/// shape again — must not park the supervisor. This subscriber deliberately <b>ignores</b> its
|
||||
/// token, so only the fan-out's own ceiling can end the wait.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Asserted at the transport, not at dispose timing: with the ceiling, each hung re-subscribe
|
||||
/// is abandoned, the session torn down, and another CONNECT reaches the broker; without it,
|
||||
/// the supervisor is parked forever on the first one and the broker never sees another. A
|
||||
/// dispose-elapsed assertion cannot tell those apart — <c>DisposeAsync</c>'s own bounded join
|
||||
/// returns promptly either way, leaving the supervisor alive behind it.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task ReSubscribeThatHangsIgnoringItsToken_DoesNotParkTheSupervisor()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
var opts = BrokerOpts(broker.Port) with
|
||||
{
|
||||
ConnectTimeoutSeconds = 2, ReconnectMinBackoffSeconds = 1, ReconnectMaxBackoffSeconds = 2,
|
||||
};
|
||||
var conn = new MqttConnection(opts, driverId: "t", logger: null);
|
||||
var entered = new TaskCompletionSource();
|
||||
conn.Reconnected += async _ =>
|
||||
{
|
||||
entered.TrySetResult();
|
||||
await Task.Delay(Timeout.InfiniteTimeSpan); // no token — only the ceiling bounds this
|
||||
};
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
broker.DropAll();
|
||||
await entered.Task.WaitAsync(TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken);
|
||||
|
||||
// The first reconnect is accept #2; a supervisor that recovered from the hang makes at least
|
||||
// one more. A parked one never does.
|
||||
(await WaitUntilAsync(() => broker.AcceptedCount >= 3, TimeSpan.FromSeconds(25)))
|
||||
.ShouldBeTrue($"the supervisor is parked behind a hung re-subscribe ({broker.AcceptedCount} connects seen)");
|
||||
|
||||
var sw = Stopwatch.StartNew();
|
||||
await conn.DisposeAsync();
|
||||
sw.Stop();
|
||||
|
||||
sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(20), "dispose was parked behind a hung re-subscribe");
|
||||
conn.State.ShouldBe(MqttConnectionState.Disposed);
|
||||
var acceptedAtDispose = broker.AcceptedCount;
|
||||
await Task.Delay(TimeSpan.FromSeconds(5), TestContext.Current.CancellationToken);
|
||||
broker.AcceptedCount.ShouldBe(acceptedAtDispose, "the supervisor outlived DisposeAsync");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// M3: on the reconnect path the socket comes up before the subscriptions do. Publishing
|
||||
/// <c>Connected</c> in that window advertises exactly the healthy-looking-but-deaf condition
|
||||
/// this type exists to prevent. It is also the documented case where <c>State</c> and
|
||||
/// <c>IsConnected</c> legitimately disagree.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task Reconnect_DoesNotPublishConnectedUntilTheReSubscribeHasCompleted()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
await using var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null);
|
||||
var inSubscriber = new TaskCompletionSource();
|
||||
var release = new TaskCompletionSource();
|
||||
conn.Reconnected += async _ =>
|
||||
{
|
||||
inSubscriber.TrySetResult();
|
||||
await release.Task;
|
||||
};
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
broker.DropAll();
|
||||
await inSubscriber.Task.WaitAsync(TimeSpan.FromSeconds(30), TestContext.Current.CancellationToken);
|
||||
|
||||
conn.IsConnected.ShouldBeTrue("the socket is up");
|
||||
conn.State.ShouldBe(MqttConnectionState.Reconnecting, "…but it has no subscriptions yet");
|
||||
|
||||
release.SetResult();
|
||||
|
||||
(await WaitUntilAsync(() => conn.State == MqttConnectionState.Connected, TimeSpan.FromSeconds(10)))
|
||||
.ShouldBeTrue();
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------------
|
||||
// helpers
|
||||
// ---------------------------------------------------------------------------------
|
||||
@@ -720,6 +949,44 @@ public sealed class MqttConnectionTests
|
||||
ReconnectMaxBackoffSeconds = 2,
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// Captures formatted log messages. The supervisor's failed-attempt path is Debug-only and has
|
||||
/// no other externally visible effect — MQTTnet refuses a connect-while-connected before
|
||||
/// opening a socket, so the broker never sees it — which is precisely why the C1 defect was
|
||||
/// silent. The log is the observable.
|
||||
/// </summary>
|
||||
private sealed class CapturingLogger : ILogger
|
||||
{
|
||||
private readonly List<string> _messages = [];
|
||||
|
||||
public int CountContaining(string fragment)
|
||||
{
|
||||
lock (_messages)
|
||||
{
|
||||
return _messages.Count(m => m.Contains(fragment, StringComparison.OrdinalIgnoreCase));
|
||||
}
|
||||
}
|
||||
|
||||
public IDisposable? BeginScope<TState>(TState state)
|
||||
where TState : notnull
|
||||
=> null;
|
||||
|
||||
public bool IsEnabled(LogLevel logLevel) => true;
|
||||
|
||||
public void Log<TState>(
|
||||
LogLevel logLevel,
|
||||
EventId eventId,
|
||||
TState state,
|
||||
Exception? exception,
|
||||
Func<TState, Exception?, string> formatter)
|
||||
{
|
||||
lock (_messages)
|
||||
{
|
||||
_messages.Add(formatter(state, exception));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private static async Task<bool> WaitUntilAsync(Func<bool> condition, TimeSpan timeout)
|
||||
{
|
||||
var deadline = Stopwatch.StartNew();
|
||||
|
||||
Reference in New Issue
Block a user