numinnex commented on code in PR #4388:
URL: https://github.com/apache/iggy/pull/4388#discussion_r4186170107
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -168,10 +173,12 @@ impl Default for TcpClient {
#[async_trait]
impl Client for TcpClient {
async fn connect(&self) -> Result<(), IggyError> {
+ self.disconnected_by_caller.store(false, Ordering::SeqCst);
Review Comment:
the leader-redirect path in `IggyClient::login_user` (binary_users.rs:119,
same in binary_personal_access_tokens.rs:80) calls the trait `connect()`
through `ClientWrapper`, so it lands here, clears the flag, and
`IggyClient::connect` respawns the heartbeat (clients/client.rs:977). A
`disconnect()` that lands while the redirect metadata RPC is in flight is
undone after `disconnect()` returns, so this isn't problem 6.
The redirect should use an intent-preserving connect (no flag clear), or
bail with `NotConnected` when the flag is set.
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -225,6 +232,9 @@ impl BinaryTransport for TcpClient {
}
async fn send_raw_with_response(&self, code: u32, payload: Bytes) ->
Result<Bytes, IggyError> {
+ if self.disconnected_by_caller.load(Ordering::SeqCst) {
Review Comment:
removing this guard (and the same one at quic :181 and ws :175) keeps all
237 lib tests green. Only the connect_inner arms are covered. A unit test needs
AutoLogin credentials, because without them `sign_in_credentials().is_none()`
(:270) masks the guard.
Also, after `shutdown()` and then `Client::disconnect`, ping now returns
`NotConnected` instead of `ClientShutdown`, against the doc at
common/traits/client.rs:61. Check `Shutdown` first, or skip the store when shut
down.
##########
core/common/src/traits/client.rs:
##########
@@ -49,6 +49,11 @@ pub trait Client:
async fn connect(&self) -> Result<(), IggyError>;
/// Disconnect from the server. If the client is not connected, it will do
nothing.
+ ///
+ /// On TCP, QUIC and WebSocket, the disconnect holds until the next
`connect()`:
+ /// requests fail with `IggyError::NotConnected`, and nothing reconnects
the
Review Comment:
typed requests that need auth go through `fail_if_not_authenticated`
(binary_auth.rs:21) and return `Disconnected`, not `NotConnected`. The PR's own
lifecycle tests assert `Disconnected` for `get_me`. Document both or unify.
Line 51's "it will do nothing" is no longer true either, because disconnect now
arms the flag on a never-connected client.
##########
core/sdk/src/clients/client.rs:
##########
@@ -994,6 +1000,10 @@ impl Client for IggyClient {
}
async fn disconnect(&self) -> Result<(), IggyError> {
+ // A heartbeat ping on a disconnected client only logs errors, and on
+ // QUIC a redial it starts during the disconnect blocks the call.
+ // `connect` starts the heartbeat again.
+ self.stop_heartbeat();
Review Comment:
`stop_heartbeat` aborts without awaiting. If the heartbeat owns a reconnect
sweep, its `ConnectOwner` is abandoned (leader_aware.rs:573) and the state
stays `Connecting`. The TCP disconnect then takes the `Connecting` arm and
returns Ok ("a connect is already in flight") with no teardown, no
`reset_vsr_session`/`clear_session` and no `Disconnected` event, so pooled
sockets stay open.
Await the aborted handle, and have the TCP trait disconnect tear down when
`Connecting && !connect_coordinator.is_active()`. Separately, a concurrent
`connect()` can respawn the heartbeat after this stop, and it then logs an
error every interval.
##########
core/integration/tests/sdk/client_lifecycle.rs:
##########
@@ -39,10 +38,9 @@ const FIRST_HEARTBEAT_SETTLE: Duration =
Duration::from_millis(500);
/// with no dial and no implicit wait, so it has to fail well within this
bound.
const SDK_SIDE_FAILURE: Duration = Duration::from_millis(500);
/// A disconnect closes local resources only, so it must return well within
-/// this bound. On QUIC, a heartbeat redial that lands inside the drain of the
old
+/// this bound. On QUIC, a redial that lands inside the drain of the old
/// connection blocks `disconnect()` in `endpoint.wait_idle()` for good, so the
Review Comment:
nit: dropping the "depends on timing" hedge makes it read as if the QUIC
`wait_idle` hang is gone, but a recovery dial can still install a connection
(quic_client.rs:808) and block. Keep the hedge or name problem 6.
##########
core/sdk/src/quic/quic_client.rs:
##########
@@ -693,6 +704,10 @@ impl QuicClient {
trace!("Client is already connecting.");
return Ok(());
}
+ _ if self.disconnected_by_caller.load(Ordering::SeqCst) => {
Review Comment:
this check doesn't cover a connect already past it. The QUIC/WS ladder
(sleep :726, install :808-814) can still install a live connection after an
explicit disconnect, for up to `reestablish_after` plus the ladder, and the
`wait_idle` hang is still reachable. On QUIC, auto-login refused by the guard
(:846) also returns with state `Authenticating` and the connection installed,
so the next `connect()` takes the `Authenticating` arm, skips sign-in and the
client is wedged.
Re-check the flag per pass and drop the new connection when it's set.
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -645,6 +656,10 @@ impl TcpClient {
trace!("Client is already connecting.");
return Ok(());
}
+ _ if self.disconnected_by_caller.load(Ordering::SeqCst) =>
{
Review Comment:
the flag is checked only here, never inside the sweep loop. If
`Client::disconnect` lands during a TCP auto-login sweep, the sweep's
`login_user` hits the send guard (:235) and gets `NotConnected`. `fail_sign_in`
reads that as connection lost (:902), sets `Connecting` (:744), sleeps and
redials, and with the default `max_retries: None` it never ends.
A temp unit test showed 15 accepts in 3s, the owner's connect future never
resolving and the state stuck at `Connecting`. Before this PR the same window
cost one reconnect. Re-check the flag before each dial, after each retry sleep
and before sign-in, then `fail_connect()` + `Err(NotConnected)`, on all three
transports.
##########
foreign/python/src/client.rs:
##########
@@ -522,12 +522,10 @@ impl IggyClient {
/// for the rest of `reestablish_after` (5 s by default). The sign-in made
/// with `login_user` is dropped, so it must be repeated after
reconnecting.
/// A client configured with auto-login credentials signs in again on
- /// `connect`. Over HTTP there is no connection to close and this call does
- /// nothing.
- ///
- /// Known issue: unless reconnection is disabled, the heartbeat of a client
- /// with auto-login credentials connects it again and signs in within one
- /// heartbeat interval. See https://github.com/apache/iggy/issues/4287.
+ /// `connect`. Until then, requests fail with `RuntimeError`, and nothing
+ /// connects the client again in the background, even with auto-login
+ /// credentials. Over HTTP there is no connection to close and this call
Review Comment:
nit: over HTTP `IggyClient::disconnect` now stops the heartbeat, so "does
nothing" isn't accurate here, in the .pyi (1352) or in iggy.hpp (3396). Either
skip `stop_heartbeat` for `ClientWrapper::Http` or adjust the docs.
##########
foreign/python/tests/test_connectivity.py:
##########
@@ -43,8 +43,8 @@
def binary_transport_configs(auto_login: AutoLogin | None = None) -> list:
"""Return a config for every transport that holds a connection.
- Auto-login is disabled by default: with credentials to replay, a ping sent
- while disconnected reconnects on its own instead of failing.
+ Auto-login is disabled by default, so a test opts in only when it checks
Review Comment:
nit: the docs now promise no background reconnect with auto-login, but no
Python test pins it. Add a case with auto-login that disconnects, expects
`RuntimeError` on ping, then reconnects successfully.
--
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]
