numinnex commented on code in PR #3944:
URL: https://github.com/apache/iggy/pull/3944#discussion_r3852356665
##########
foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.cs:
##########
@@ -1103,6 +1134,17 @@ private async Task TryEstablishConnectionAsync(bool
autoLogin, CancellationToken
throw;
}
+ // Every other endpoint gets its turn before the retry delay:
the node just lost may be gone for
+ // good, and pausing on it helps nothing.
+ if (++candidate < candidates.Length)
Review Comment:
Moved both checks down, next to `retryCount++`, so they gate a rotation
rather than a dial.
##########
foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.cs:
##########
@@ -1206,18 +1296,29 @@ private async Task<IMemoryOwner<byte>>
SendWithResponseAsync(int code, ReadOnlyM
catch (Exception e) when (IsLostConnection(e) && !IsConnecting &&
!_disposed)
{
_logger.LogWarning("Connection lost");
+
+ // A server-side eviction is the server ending this session
authoritatively, like a logout: the
+ // remembered sign-in ends with it, so only a configured auto
login may bring the session back.
+ // Remembered credentials exist for transport loss, where the
session died with the socket rather
+ // than by anyone's decision.
+ if (e is IggyInvalidStatusCodeException { StatusCode:
VsrError.STALE_CLIENT, FromServer: true })
Review Comment:
Fixed, in the consensus layer rather than the catch, so it does not depend
on how the eviction is reported: `TcpMessageStream.Vsr.cs` calls
`ForgetSessionAfterEviction(evicted.Verdict)` in the
`VsrSessionEvictedException` branch, before deciding whether to wrap it in
`VsrRequestOutcomeUnknownException`.
I deliberately did not widen `IsLostConnection` to match the wrapper: that
would make a replicated write with an unknown outcome eligible for the
reconnect-and-replay path, which is the one thing
`VsrRequestOutcomeUnknownException` exists to prevent.
New test `ServerEvictionDuringAReplicatedWriteForgetsTheRememberedSignIn`:
the eviction lands on a `CreateStream`, the caller gets
`VsrRequestOutcomeUnknownException`, and the next request fails
`NotConnectedException` with no new connection. Red without the fix.
##########
foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.cs:
##########
@@ -82,6 +82,17 @@ public sealed partial class TcpMessageStream : IIggyClient
private DateTimeOffset _lastConnectionTime;
private int _stateValue = (int)ConnectionState.Disconnected;
+ // Every node the roster named on the last read, kept as dial candidates.
A node dies together with its
+ // address, and the roster is unreachable exactly when it is needed, so
the client has to have remembered it
+ // while the connection was still healthy. Written by the leader probe,
read by the connect loop.
+ private string[] _rosterAddresses = [];
+
+ // The credentials a sign-in succeeded with, so a reconnect - on this node
or, after a failover, another one -
+ // can re-establish the session instead of leaving every later request
unauthenticated. A caller that signs in
+ // by hand is otherwise less reconnectable than one that configures auto
login, which is a surprising
+ // difference between two ways of doing the same thing. Cleared on
sign-out and on a server eviction.
+ private AutoLoginSettings? _rememberedLogin;
Review Comment:
Fixed: `Dispose()` nulls it, so the plain-string password or token does not
outlive the client.
##########
foreign/csharp/Iggy_SDK/IggyClient/Implementations/TcpMessageStream.cs:
##########
@@ -725,6 +744,9 @@ public async Task LogoutUserAsync(CancellationToken token =
default)
{
await ResetConsensusSessionAsync();
+ // An explicit sign-out leaves no session to restore, and a
reconnect must not resurrect one.
Review Comment:
Comment rewritten to claim only what the code does: the sign-in this client
remembered does not outlive an explicit sign-out, while credentials configured
as `AutoLoginSettings` are a different promise — they are what every connect
signs in with — and a later reconnect still uses them.
##########
foreign/csharp/Iggy_SDK_Tests/VsrTests/DialCandidatesTests.cs:
##########
@@ -0,0 +1,64 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+using Apache.Iggy.IggyClient.Implementations;
+
+namespace Apache.Iggy.Tests.VsrTests;
+
+/// <summary>
+/// Mirrors the Rust SDK's <c>dial_candidates</c>: a client that loses the
node it is on has to dial the rest
+/// of the cluster, and the two SDKs have to agree on which endpoints
those are and in what order.
+/// </summary>
+public sealed class DialCandidatesTests
+{
+ [Fact]
+ public void LeadsWithTheCurrentEndpointThenNamesEachOtherOneOnce()
+ {
+ var candidates = TcpMessageStream.DialCandidates(
Review Comment:
Added `DialsTheConfiguredAddressBeforeTheLearnedRoster`: a distinct current
endpoint, a distinct base address and a two-node roster, asserting the full
order. Swapping `Prepend` for `Append` fails it.
##########
foreign/csharp/Iggy_SDK_Tests/VsrTests/EndpointFailoverTests.cs:
##########
@@ -0,0 +1,428 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+using System.Buffers.Binary;
+using System.Net;
+using System.Net.Sockets;
+using System.Text;
+using Apache.Iggy.Configuration;
+using Apache.Iggy.Contracts.Tcp;
+using Apache.Iggy.Enums;
+using Apache.Iggy.IggyClient.Implementations;
+using Microsoft.Extensions.Logging.Abstractions;
+
+namespace Apache.Iggy.Tests.VsrTests;
+
+/// <summary>
+/// The node a client signed in on dies; its next request has to complete
on a survivor the roster named,
+/// under a session established there. Mirrors
+/// <c>core/integration/tests/cluster/failover_client_continuity.rs</c>.
+/// </summary>
+public sealed class EndpointFailoverTests
+{
+ private const int HeaderSize = 256;
+ private const int SizeOffset = 48;
+ private const int CommandOffset = 60;
+ private const int RequestIdOffset = 168;
+ private const int RequestOperationOffset = 176;
+ private const int RequestReservedOffset = 196;
+ private const int ReplyRequestIdOffset = 200;
+ private const int ReplyOperationOffset = 208;
+ private const int ReplyStatusOffset = 216;
+
+ private const byte CommandReply = 8;
+ private const byte CommandEviction = 13;
+ private const int EvictionReasonOffset = 255;
+ private const byte EvictionStaleClient = 13;
+ private const byte OperationRegister = 1;
+ private const byte OperationNonReplicated = 2;
+ private const int GetClusterMetadataCode = 12;
+ private const int PingCode = 1;
+
+ [Fact]
+ public async Task ResumesOnASurvivorAfterTheSignedInNodeDies()
+ {
+ using var primary = new MockNode();
+ using var survivor = new MockNode();
+
+ // The primary leads, so the sign-in settles there and the roster is
only remembered - not acted on -
+ // until the node dies.
+ primary.Serve(request => request.Code == GetClusterMetadataCode
+ ? Reply(OperationNonReplicated, ClusterMetadata(primary.Port,
survivor.Port, primary.Port))
+ : Answer(request));
+ survivor.Serve(request => request.Code == GetClusterMetadataCode
+ ? Reply(OperationNonReplicated, ClusterMetadata(primary.Port,
survivor.Port, survivor.Port))
+ : Answer(request));
+
+ var configuration = new IggyClientConfigurator
+ {
+ BaseAddress = $"127.0.0.1:{primary.Port}",
+ Protocol = Protocol.Tcp,
+ ReconnectionSettings = new ReconnectionSettings
+ {
+ Enabled = true,
+ MaxRetries = 4,
+ InitialDelay = TimeSpan.FromMilliseconds(20)
+ }
+ };
+ using var client = new TcpMessageStream(configuration,
NullLoggerFactory.Instance);
+
+ await client.ConnectAsync(TestContext.Current.CancellationToken);
+ // No auto login: the credentials come from the caller's own sign-in,
which is the shape that could not
+ // reconnect at all before.
+ await client.LoginUserAsync("iggy", "iggy",
TestContext.Current.CancellationToken);
+ await client.PingAsync(TestContext.Current.CancellationToken);
+ Assert.Equal(1, primary.Pings);
+
+ primary.Kill();
+
+ // The request in flight when the node died is allowed to fail; what
is not allowed is never completing
+ // one, which is what a client that only knows the dead endpoint does.
+ var (resumed, lastError) = await ResumedWithin(client,
TimeSpan.FromSeconds(10));
+ Assert.True(resumed,
+ $"the client has to resume on the survivor the roster named
({lastError}, survivor saw " +
+ $"{survivor.Registrations} registrations and {survivor.Pings}
pings)");
+ Assert.True(survivor.Registrations >= 1, "the remembered credentials
signed in again on the survivor");
+ Assert.True(survivor.Pings >= 1, "the request landed on the survivor");
+ }
+
+ /// <summary>
+ /// Mirrors the integration contract (HeartbeatTests
+ /// EvictedClient_WithoutAutoLogin_Should_FailFast_And_NotReconnect):
a server-side eviction ends the
+ /// session authoritatively, so the credentials a manual sign-in
remembered must not resurrect it - the
+ /// evicted request surfaces the loss with no reconnect attempt.
+ /// </summary>
+ [Fact]
+ public async Task ServerEvictionForgetsTheRememberedSignIn()
+ {
+ using var node = new MockNode();
+ var evict = false;
+ node.Serve(request =>
+ {
+ if (request.Operation == OperationRegister)
+ {
+ return Reply(OperationRegister, RegisterBody(session: 128));
+ }
+
+ return evict
+ ? EvictionFrame(EvictionStaleClient)
+ : Reply(OperationNonReplicated, request.Code ==
GetClusterMetadataCode
+ ? ClusterMetadata(node.Port, node.Port, node.Port)
+ : []);
+ });
+
+ var configuration = new IggyClientConfigurator
+ {
+ BaseAddress = $"127.0.0.1:{node.Port}",
+ Protocol = Protocol.Tcp,
+ ReconnectionSettings = new ReconnectionSettings
+ {
+ Enabled = true,
+ MaxRetries = 2,
+ InitialDelay = TimeSpan.FromMilliseconds(20)
+ }
+ };
+ using var client = new TcpMessageStream(configuration,
NullLoggerFactory.Instance);
+
+ await client.ConnectAsync(TestContext.Current.CancellationToken);
+ await client.LoginUserAsync("iggy", "iggy",
TestContext.Current.CancellationToken);
+ await client.PingAsync(TestContext.Current.CancellationToken);
+ var connectionsBeforeEviction = node.Connections;
+
+ evict = true;
+ await Assert.ThrowsAnyAsync<Exception>(() =>
client.PingAsync(TestContext.Current.CancellationToken));
Review Comment:
Done: `IggyInvalidStatusCodeException` with `StatusCode ==
VsrError.STALE_CLIENT` and `FromServer` asserted, then `NotConnectedException`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
