numinnex commented on code in PR #3944:
URL: https://github.com/apache/iggy/pull/3944#discussion_r3852352355
##########
core/common/src/traits/binary_impls/users.rs:
##########
@@ -218,6 +218,11 @@ impl<B: BinaryClient> UserClient for B {
"authenticated against iggy server"
);
self.set_state(ClientState::Authenticated).await;
+ self.remember_session_credentials(Credentials::UsernamePassword(
Review Comment:
Fixed. `VsrSessionControl` grew `refresh_session_password(user,
new_password)`, which `change_password` calls after the change commits. The TCP
client remembers the `user_id` the login returned alongside the credentials,
and swaps the password in only when the change targets that user — matched by
numeric id or by the remembered username, since `change_password` takes either.
A remembered personal access token is left alone: it is not derived from the
password.
Three tests: same user via both identifier forms, another user (numeric and
named), and a remembered token.
##########
core/integration/tests/sdk/disconnect_relogin.rs:
##########
@@ -0,0 +1,65 @@
+// 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.
+
+//! An explicit disconnect ends the session for good: the credentials a manual
+//! sign-in remembered (for reconnecting across involuntary drops and
+//! failovers) must not resurrect it. Pins at the Rust layer the contract the
+//! C++ e2e suite asserts through the FFI
(`DisconnectThenReconnectWithoutRelogin`,
+//! `GetStatsBeforeLoginThrows`), so a regression fails here first instead of
+//! three suites downstream.
+
+use iggy::prelude::*;
+use integration::iggy_harness;
+
+#[iggy_harness]
+async fn
given_a_logged_in_client_when_explicitly_disconnected_should_require_a_fresh_login(
+ harness: &TestHarness,
+) {
+ let client = harness.new_client().await.unwrap();
+ client
+ .login_user(DEFAULT_ROOT_USERNAME, DEFAULT_ROOT_PASSWORD)
+ .await
+ .unwrap();
+ client.get_me().await.expect("authenticated get_me works");
+
+ client.disconnect().await.unwrap();
+ client.connect().await.unwrap();
+ assert!(
+ client.get_me().await.is_err(),
Review Comment:
Done: `Err(IggyError::Unauthenticated)` at the first assertion and
`Err(IggyError::NotConnected)` at the second.
##########
core/sdk/src/leader_aware.rs:
##########
@@ -162,7 +217,7 @@ fn process_cluster_metadata(
/// Check if two addresses refer to the same endpoint
/// Handles various formats like 127.0.0.1:8090 vs localhost:8090
-fn is_same_address(addr1: &str, addr2: &str) -> bool {
+pub(crate) fn is_same_address(addr1: &str, addr2: &str) -> bool {
Review Comment:
Fixed. `is_same_address` now falls back to resolution when the spellings
differ and at least one side is not a literal socket address: two literals that
differ are still unequal without a lookup, and a name the resolver does not
know compares unequal as before, costing at worst the extra dial it costs
today. Test `a_host_name_matches_the_address_it_resolves_to`.
The Java SDK deliberately went the other way for its redial dedup (see the
`resolveToSameHost` thread): that path runs on the Netty event loop, so it
compares spellings only and keeps the resolving comparison for the leader
check. The Rust connect path already blocks on the network, so resolving there
is free.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -1025,16 +1063,16 @@ func (c *IggyTcpClient) connectionCandidates() []string
{
// backup: once the caller signs in, the first replicated request fails over
// through the transient-deny path.
func (c *IggyTcpClient) establishSession(ctx context.Context, skipAutoLogin
bool) error {
- if !c.config.autoLogin.enabled {
- c.logger.Info("Automatic sign-in is disabled.")
+ credentials, ok := c.signInCredentials()
Review Comment:
Fixed at the source rather than by swallowing alone. `endBoundSession` now
issues the logout connect-scoped, so `exchange` returns the error instead of
entering the reconnect path — the deadlock happens inside `LogoutUser`, before
it ever returns, so swallowing on its own would not have been enough. A
reconnectable failure is then swallowed and the local session state reset,
because a session whose logout could not be delivered died with its socket and
the server fences what it left behind. The sign-in that follows replays through
its own reconnect.
`TestFailover_ReLoginSurvivesALogoutTheTransportSwallowed` reproduces your
fake server (the handler drops the logout frame and ends the connection) and
fails after 15s against the old code.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -994,6 +991,47 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error
{
return nil
}
+// dialCandidate opens one connection, wrapping it in TLS when configured, and
+// records the endpoint that answered: the leader check compares against it and
+// the next reconnect starts from it.
+func (c *IggyTcpClient) dialCandidate(ctx context.Context, address string)
(net.Conn, error) {
+ c.logger.Info("Iggy client is connecting to server...",
slog.String("server_address", address))
+ connection, err := (&net.Dialer{}).DialContext(ctx, "tcp", address)
Review Comment:
Fixed. `dialCandidate` takes a `bounded` flag and, when more than one
candidate is queued, derives a `context.WithTimeout(ctx, failoverDialTimeout)`
used for both `DialContext` and `HandshakeContext` — the handshake had no
deadline either, and a peer that accepts TCP and never answers is the same
failure. `failoverDialTimeout` is 2s, like Rust.
`TestFailover_BoundsTheDialWhenOtherEndpointsAreQueuedBehindIt` pins the
constant and that a listener which accepts and never answers cannot hold the
sweep.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -480,12 +488,22 @@ func (c *IggyTcpClient) exchange(ctx context.Context,
code uint32, frame []byte)
return nil, err
}
- // Without auto-login a reconnect cannot restore the session, so
anything
- // but a sign-in fails here instead of replaying unauthenticated. The
- // sign-in itself is the exception: the server stays silent on a
transient
+ // A stale-client 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 errors.Is(err, ierror.ErrStaleClient) {
Review Comment:
Moved right after the `isReconnectable` check, above every gate that returns.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -923,46 +943,23 @@ func (c *IggyTcpClient) Connect(ctx context.Context)
error {
}),
).Do(
func() error {
- address := candidates[candidateIndex%len(candidates)]
- candidateIndex++
- c.logger.Info("Iggy client is connecting to server...",
slog.String("server_address", address))
- connection, err := (&net.Dialer{}).DialContext(ctx,
"tcp", address)
- if err != nil {
- c.logger.Error("Failed to establish TCP
connection to the server", slog.Any("error", err))
- return ierror.ErrCannotEstablishConnection
- }
-
- tc := connection.(*net.TCPConn)
- if err := tc.SetNoDelay(c.config.noDelay); err != nil {
- c.logger.Error("Failed to set the nodelay
option on the client, continuing...", slog.Any("error", err))
- }
-
- c.mtx.Lock()
- c.clientAddress = tc.LocalAddr().String()
- c.currentServerAddress = address
- c.mtx.Unlock()
+ // Every endpoint gets its turn inside one attempt, so
a full pass
+ // over the cluster costs one retry rather than one per
endpoint:
+ // a pass that stopped at the first refusal would never
reach the
+ // survivors of a client configured for a single retry.
+ var lastErr error
+ for _, address := range candidates {
Review Comment:
Fixed. `Connect` now rejects an empty candidate list with
`ErrCannotEstablishConnection` and puts the transport state back to
`TransportStateDisconnected`, so nothing reports connected with a nil conn.
`TestFailover_RejectsAConnectWithNoEndpointToDial` builds a client with
`WithServerAddress("")` and asserts both the error and that a request still
fails.
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -994,6 +991,47 @@ func (c *IggyTcpClient) Connect(ctx context.Context) error
{
return nil
}
+// dialCandidate opens one connection, wrapping it in TLS when configured, and
+// records the endpoint that answered: the leader check compares against it and
+// the next reconnect starts from it.
+func (c *IggyTcpClient) dialCandidate(ctx context.Context, address string)
(net.Conn, error) {
+ c.logger.Info("Iggy client is connecting to server...",
slog.String("server_address", address))
+ connection, err := (&net.Dialer{}).DialContext(ctx, "tcp", address)
+ if err != nil {
+ c.logger.Error("Failed to establish TCP connection to the
server", slog.Any("error", err))
+ return nil, ierror.ErrCannotEstablishConnection
+ }
+
+ tc := connection.(*net.TCPConn)
+ if err := tc.SetNoDelay(c.config.noDelay); err != nil {
+ c.logger.Error("Failed to set the nodelay option on the client,
continuing...", slog.Any("error", err))
+ }
+
+ c.mtx.Lock()
+ c.clientAddress = tc.LocalAddr().String()
+ c.currentServerAddress = address
Review Comment:
Moved after the handshake: `dialCandidate` builds the connection, wraps it
in TLS when configured, and only then records `clientAddress` /
`currentServerAddress`.
`TestFailover_DoesNotSettleOnAnEndpointThatFailedTheHandshake` (dead configured
address, a plaintext listener in the roster, TLS on) asserts the current
address stays where it was.
--
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]
