Skip to content

Commit 204bd6e

Browse files
AndyTWFclaude
andcommitted
Reinitialise on connect, bound the auth callback, and complete the teardown
RTN11d - fixed. Only the channel half ran, so Connection.errorReason, msgSerial and the channel's channelSerial all survived into the new connection. RTN11b - fixed. connect() while CLOSING applied no part of RTN11d. RTN12b - fixed. The close timeout was a hardcoded 1s, not realtimeRequestTimeout, so on a slow link close() could reach CLOSED unacknowledged. TO3l11 - implemented. realtimeRequestTimeout was internal and could not be set. Validated at both ends; the upper bound is Int32.MaxValue ms, the tightest limit across every timer sink on every framework this package ships. RSA4c - fixed. The bound was applied to the task the callback returned, so a callback whose body runs synchronously was never bounded at all. RSA4c1 - fixed. cause was set as InnerException, which is not the spec's field. RTN8d, RTN9d - fixed. A throwing transition skipped the key clear and the transport teardown, leaving a terminal state holding a resumable key and a live transport. Also carries RealtimeWorkflowSpecs.cs in full: its additions are one contiguous insertion of complete test classes and cannot be split across the commits whose spec points they cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 29c9833 commit 204bd6e

9 files changed

Lines changed: 1466 additions & 32 deletions

File tree

src/IO.Ably.Shared/AblyAuth.cs

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -305,11 +305,27 @@ public virtual async Task<TokenDetails> RequestTokenAsync(TokenParams tokenParam
305305
bool shouldCatch = true;
306306
try
307307
{
308-
var callbackResult = await authOptions.AuthCallback(tokenParams);
308+
// RSA4c bounds an auth attempt by realtimeRequestTimeout. This one is awaited on
309+
// the realtime workflow's single reader thread, so an authCallback that never
310+
// returns would stall channel state, inbound messages and the idle monitor.
311+
//
312+
// Task.Run rather than awaiting the delegate directly: TimeoutAfter extends an
313+
// already-created Task, so a callback whose body runs synchronously - the most
314+
// ordinary shape in C# - would block before there is anything to bound. It
315+
// cannot cancel the callback; an abandoned one runs to completion.
316+
var callbackResult = await Task.Run(() => authOptions.AuthCallback(tokenParams))
317+
.TimeoutAfter(Options.RealtimeRequestTimeout, null);
309318

310319
if (callbackResult == null)
311320
{
312-
throw new AblyException("AuthCallback returned null", ErrorCodes.ClientAuthProviderRequestFailed);
321+
// Covers a callback that returned null and one that timed out - TimeoutAfter
322+
// yields null for both, and the wrapper below replaces the message anyway.
323+
// ClientCallbackError, not ClientAuthProviderRequestFailed: the wrapper
324+
// rethrows as 80019 per RSA4c1 with this as the cause, so 80019 here would
325+
// duplicate its own parent. ably-js uses 40170 for the same case.
326+
throw new AblyException(
327+
$"AuthCallback returned null or did not return within {Options.RealtimeRequestTimeout.TotalSeconds}s",
328+
ErrorCodes.ClientCallbackError);
313329
}
314330

315331
if (callbackResult is TokenDetails)
@@ -341,12 +357,17 @@ public virtual async Task<TokenDetails> RequestTokenAsync(TokenParams tokenParam
341357
: HttpStatusCode.Unauthorized;
342358
}
343359

360+
// RSA4c1 wants the cause "set to the underlying cause". The four argument overload
361+
// assigns InnerException instead, which is not the spec's field and is not
362+
// serialised as one.
344363
throw new AblyException(
345364
new ErrorInfo(
346365
"Error calling AuthCallback, token request failed. See inner exception for details.",
347366
ErrorCodes.ClientAuthProviderRequestFailed,
348367
statusCode,
349-
ex),
368+
href: null,
369+
cause: (ex as AblyException)?.ErrorInfo ?? new ErrorInfo(ex.Message),
370+
innerException: ex),
350371
ex);
351372
}
352373
}

src/IO.Ably.Shared/AuthOptions.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,13 @@ public class AuthOptions
1313
/// <summary>
1414
/// The callback used to get a new <see cref="IO.Ably.TokenDetails"/> or <see cref="IO.Ably.TokenRequest"/>.
1515
/// AuthCallback is used by internally by <see cref="IO.Ably.AblyAuth"/>.RequestTokenAsync.
16+
/// <para>
17+
/// The callback is bounded by <see cref="ClientOptions.RealtimeRequestTimeout"/> per RSA4c,
18+
/// but cannot be cancelled: one that overruns is abandoned and keeps running, so a later auth
19+
/// attempt may invoke it again concurrently and implementations must tolerate that. A result
20+
/// returned after the bound is discarded, and each abandoned invocation holds a thread pool
21+
/// worker until it returns.
22+
/// </para>
1623
/// </summary>
1724
public Func<TokenParams, Task<object>> AuthCallback { get; set; }
1825

src/IO.Ably.Shared/ClientOptions.cs

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,51 @@ public bool UseBinaryProtocol
295295
/// </summary>
296296
public TimeSpan ChannelRetryTimeout { get; set; } = Defaults.ChannelRetryTimeout;
297297

298+
private TimeSpan _realtimeRequestTimeout = Defaults.RealtimeRequestTimeout;
299+
300+
/// <summary>
301+
/// How long the library waits for Ably to answer before treating a realtime request as
302+
/// having failed. Applies while establishing a connection, while awaiting a response to a
303+
/// Heartbeat, Connect, Attach, Detach or Close, and as part of the RTN23a idle timeout.
304+
/// Default: 10s. Must be a positive interval, and values beyond a minute or so are rarely
305+
/// useful. Disabling the timeout is not supported - the timeouts this value drives are all
306+
/// required to fire - so both Timeout.InfiniteTimeSpan and TimeSpan.MaxValue are rejected.
307+
/// TO3l11 - https://sdk.ably.com/builds/ably/specification/main/features/#TO3l11.
308+
/// </summary>
309+
/// <exception cref="ArgumentOutOfRangeException">when set to zero, a negative value, or an
310+
/// interval too large for the underlying timers to fire.</exception>
311+
public TimeSpan RealtimeRequestTimeout
312+
{
313+
get => _realtimeRequestTimeout;
314+
315+
set
316+
{
317+
// A non-positive value fails quietly rather than loudly: zero turns the RTN14c
318+
// connect timeout into a hot reconnect loop, and a negative one stops the timer
319+
// firing at all. Timeout.InfiniteTimeSpan is -1ms, so the lower bound catches it -
320+
// which matters, because both Task.Delay and System.Threading.Timer accept it as
321+
// genuine infinity and it would disable RTN14c, RTN12b, RTL4f, RTL5f and RSA4c.
322+
//
323+
// The upper bound is the tightest limit across the sinks this value reaches on every
324+
// framework shipped: Task.Delay allows uint.MaxValue - 1 ms on .NET 6+ but only
325+
// Int32.MaxValue on .NET Framework, Mono and Xamarin, and CountdownTimer casts to
326+
// int for System.Threading.Timer. So Int32.MaxValue ms - a bound on the arithmetic,
327+
// not a supported configuration, which is why the message names the useful range.
328+
if (value <= TimeSpan.Zero || value.TotalMilliseconds > int.MaxValue)
329+
{
330+
throw new ArgumentOutOfRangeException(
331+
nameof(RealtimeRequestTimeout),
332+
value,
333+
"RealtimeRequestTimeout must be a positive interval. The default is 10s; " +
334+
"values beyond a minute or so are rarely useful. Disabling the timeout is " +
335+
"not supported - the connect, close, attach, detach and auth timeouts it " +
336+
"drives are all required to fire.");
337+
}
338+
339+
_realtimeRequestTimeout = value;
340+
}
341+
}
342+
298343
/// <summary>
299344
/// Timeout for opening an http request.
300345
/// Default: 4s.
@@ -436,8 +481,6 @@ internal Func<DateTimeOffset> NowFunc
436481

437482
internal bool SkipInternetCheck { get; set; }
438483

439-
internal TimeSpan RealtimeRequestTimeout { get; set; } = Defaults.RealtimeRequestTimeout;
440-
441484
/// <summary>
442485
/// Default constructor for ClientOptions.
443486
/// </summary>

src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs

Lines changed: 92 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,17 @@ private async Task<RealtimeCommand> ProcessCommandInner(RealtimeCommand command)
360360
switch (command)
361361
{
362362
case ConnectCommand _:
363+
364+
// RTN11d - connect() out of CLOSED or FAILED starts afresh. The channel half,
365+
// back to INITIALIZED with errorReason unset, is done per channel by the command
366+
// queued below; Id and Key are already emptied on entering CLOSED or FAILED.
367+
if (State.Connection.State == ConnectionState.Closed ||
368+
State.Connection.State == ConnectionState.Failed)
369+
{
370+
State.Connection.ErrorReason = null;
371+
State.Connection.MessageSerial = 0;
372+
}
373+
363374
var nextCommand = ConnectionManager.Connect();
364375
var initFailedChannelsOnConnect =
365376
ChannelCommand.CreateForAllChannels(InitialiseFailedChannelsOnConnect.Create().TriggeredBy(command));
@@ -949,14 +960,36 @@ private async Task<RealtimeCommand> HandleSetStateCommand(RealtimeCommand comman
949960

950961
case SetFailedStateCommand cmd:
951962

952-
ClearAckQueueAndFailMessages(ErrorInfo.ReasonFailed);
953-
954963
var error = TransformIfTokenErrorAndNotRetryable();
955964
var failedState = new ConnectionFailedState(ConnectionManager, error, Logger);
956-
SetState(failedState);
957-
State.Connection.ClearKeyAndId(); // RTN8c, RTN9c
958965

959-
ConnectionManager.DestroyTransport();
966+
// RTN7e - the queued messages are failed with "an error representing the
967+
// reason for the state change", taken off the state object so it is this
968+
// transition's reason even if SetState early-returns. In the finally, after
969+
// the transition, so a publisher's callback sees the state it is being told
970+
// about and a throwing transition cannot strand the messages uncalled.
971+
// ably-js orders it the same way: enactStateChange then failQueuedMessages.
972+
//
973+
// RTN7e - the queued messages are failed with "an error representing the
974+
// reason for the state change", taken off the state object so it is this
975+
// transition's reason even if SetState early-returns. In the finally, after
976+
// the transition, so a publisher's callback sees the state it is being told
977+
// about and a throwing transition cannot strand the messages uncalled.
978+
// ably-js orders it the same way: enactStateChange then failQueuedMessages.
979+
//
980+
// RTN8d and RTN9d share that finally: the connection has entered the state
981+
// by the time SetState rethrows, so a throw must not leave it reporting a
982+
// terminal state while still holding a resumable key and a live transport.
983+
try
984+
{
985+
SetState(failedState);
986+
}
987+
finally
988+
{
989+
ClearAckQueueAndFailMessages(failedState.Error);
990+
State.Connection.ClearKeyAndId(); // RTN8d, RTN9d
991+
ConnectionManager.DestroyTransport();
992+
}
960993

961994
ErrorInfo TransformIfTokenErrorAndNotRetryable()
962995
{
@@ -1108,7 +1141,7 @@ async Task<bool> CheckInstantRetryFlag()
11081141

11091142
var closingState = new ConnectionClosingState(ConnectionManager, connectedTransport, Logger);
11101143
SetState(closingState);
1111-
State.Connection.ClearKeyAndId(); // RTN8c, RTN9c
1144+
State.Connection.ClearKeyAndId(); // RTN8d, RTN9d
11121145

11131146
if (connectedTransport)
11141147
{
@@ -1126,27 +1159,49 @@ async Task<bool> CheckInstantRetryFlag()
11261159
State.Connection.ClearKey();
11271160
}
11281161

1129-
ClearAckQueueAndFailMessages(ErrorInfo.ReasonSuspended);
1130-
11311162
var suspendedState = new ConnectionSuspendedState(ConnectionManager, cmd.Error, Logger);
1132-
SetState(suspendedState);
1133-
State.Connection.ClearKeyAndId(); // RTN8c, RTN9c
1163+
1164+
// RTN7e and the teardown - see the note on the FAILED case.
1165+
try
1166+
{
1167+
SetState(suspendedState);
1168+
}
1169+
finally
1170+
{
1171+
ClearAckQueueAndFailMessages(suspendedState.Error);
1172+
1173+
// Deliberately NOT RTN8d/RTN9d, which name only CLOSED, CLOSING and
1174+
// FAILED. Clearing here is this library's pre-6.1.0 behaviour, kept
1175+
// because it is coupled to the connectionStateTtl freshness check that
1176+
// also predates 6.1.0.
1177+
State.Connection.ClearKeyAndId();
1178+
1179+
// Needed here as well as in the DISCONNECTED handler, which diverts to
1180+
// this case before reaching its own DestroyTransport. A surviving
1181+
// transport keeps its listener for up to suspendedRetryTimeout.
1182+
ConnectionManager.DestroyTransport();
1183+
}
11341184

11351185
break;
11361186

11371187
case SetClosedStateCommand cmd:
11381188

1139-
ClearAckQueueAndFailMessages(ErrorInfo.ReasonClosed);
1140-
11411189
var closedState = new ConnectionClosedState(ConnectionManager, cmd.Error, Logger)
11421190
{
11431191
Exception = cmd.Exception,
11441192
};
11451193

1146-
SetState(closedState);
1147-
State.Connection.ClearKeyAndId(); // RTN8c, RTN9c
1148-
1149-
ConnectionManager.DestroyTransport();
1194+
// RTN7e and the teardown - see the note on the FAILED case.
1195+
try
1196+
{
1197+
SetState(closedState);
1198+
}
1199+
finally
1200+
{
1201+
ClearAckQueueAndFailMessages(closedState.Error);
1202+
State.Connection.ClearKeyAndId(); // RTN8d, RTN9d
1203+
ConnectionManager.DestroyTransport();
1204+
}
11501205

11511206
break;
11521207
}
@@ -1177,6 +1232,8 @@ public void SetState(ConnectionStateBase newState, bool skipTimer = false)
11771232
Logger.Debug(message);
11781233
}
11791234

1235+
var notified = false;
1236+
11801237
try
11811238
{
11821239
if (newState.IsUpdate == false)
@@ -1204,13 +1261,28 @@ public void SetState(ConnectionStateBase newState, bool skipTimer = false)
12041261
Logger.Debug($"xx {newState.State}: Skipping attaching.");
12051262
}
12061263

1264+
notified = true;
12071265
UpdateStateAndNotifyConnection(newState);
12081266
}
1209-
catch (AblyException ex)
1267+
catch (Exception ex)
12101268
{
1211-
Logger.Error("Error attaching to context", ex);
1212-
1213-
UpdateStateAndNotifyConnection(newState);
1269+
// Everything, not just AblyException: anything else thrown by StartTimer or the
1270+
// state object would reach the command loop, which logs and drops it, leaving the
1271+
// connection with no transport, no timer and no state change emitted. The transition
1272+
// is still completed below and the exception still rethrown.
1273+
Logger.Error($"Error attaching to context while changing state to {newState.State}", ex);
1274+
1275+
// Only if the notify has not already happened. A throw during the transition lands
1276+
// here after the state change has been emitted - StartTimer is one source, and a
1277+
// negative retry timeout reaches System.Threading.Timer. Not a channel's
1278+
// ConnectionStateChanged, which RealtimeChannels guards per channel. Re-emitting is harmless for an ordinary transition, which the
1279+
// same-state check swallows, but an RTN24 update has no such check and was emitted
1280+
// twice. The flag is set before the call so a throw from inside it does not trigger
1281+
// a second attempt either.
1282+
if (notified == false)
1283+
{
1284+
UpdateStateAndNotifyConnection(newState);
1285+
}
12141286

12151287
newState.AbortTimer();
12161288

src/IO.Ably.Shared/Transport/ConnectionManager.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,14 @@ internal async Task OnAuthUpdated(TokenDetails tokenDetails, bool wait)
172172
{
173173
while (true)
174174
{
175-
var (success, newState) = await waiter.Wait(Defaults.RealtimeRequestTimeout);
175+
// Options rather than Defaults, since TO3l11 makes realtimeRequestTimeout a
176+
// client option.
177+
var (success, newState) = await waiter.Wait(Options.RealtimeRequestTimeout);
176178
if (success == false)
177179
{
178180
throw new AblyException(
179181
new ErrorInfo(
180-
$"Connection state didn't change after Auth updated within {Defaults.RealtimeRequestTimeout}",
182+
$"Connection state didn't change after Auth updated within {Options.RealtimeRequestTimeout}",
181183
40140));
182184
}
183185

src/IO.Ably.Shared/Transport/States/Connection/ConnectionClosingState.cs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ internal class ConnectionClosingState : ConnectionStateBase
1010
{
1111
public override ErrorInfo DefaultErrorInfo => ErrorInfo.ReasonClosed;
1212

13-
private const int CloseTimeout = 1000;
1413
private readonly bool _connectedTransport;
1514
private readonly ICountdownTimer _timer;
1615

@@ -62,7 +61,9 @@ public override void StartTimer()
6261
{
6362
if (_connectedTransport)
6463
{
65-
_timer.Start(TimeSpan.FromMilliseconds(CloseTimeout), OnTimeOut);
64+
// RTN12b - the wait for the CLOSED message is realtimeRequestTimeout, which TO3l11
65+
// makes a client option.
66+
_timer.Start(Context.DefaultTimeout, OnTimeOut);
6667
}
6768
}
6869

0 commit comments

Comments
 (0)