chengxilo commented on issue #3650:
URL: https://github.com/apache/iggy/issues/3650#issuecomment-4948748491
> Should caller-driven context.Canceled explicitly suppress automatic
reconnect, while timeouts and transport I/O errors trigger it?
Yes, exactly. In my design, when context.Canceled / context.DeadlineExceeded
fires, we stop the request and invalidate the connection, but do not reconnect
for that call: both context errors are caller-driven, so they take priority
over the client's reconnection/auto-login settings. The reconnection/auto-login
configuration only governs the TCP connection lifecycle; it never overrides a
per-request timeout. The connection is repaired on the next attempt instead:
the following request (or heartbeat ping) fails with a not-connected error,
which is retryable and triggers the reconnect. So an idle client still heals
through the heartbeat.
For comparison, the Rust SDK (which already implements reconnection) does:
send request → if the error is retryable, reconnect → send the same request
again:
https://github.com/apache/iggy/blob/f238b8cdc542992589e588ae39b3d2b56496e8e1/core/sdk/src/tcp/tcp_client.rs#L150-L209
After reviewing it again, I don't think this part is right: the Rust SDK
replays the request unconditionally, even when it is not idempotent (e.g. send
messages) and the server may have already executed it. (Thank you for your
comment, without it I probably won't notice it) The possible Iggy error that
would cause it include, ` IggyError::EmptyResponse`, `IggyError::Disconnected`
and `IggyError::TcpError`. However, I am not sure how to fix this yet, probably
a discussion would be needed with Rust developer.
Additionally, with the current Rust SDK, when the network fails under
concurrent requests, each failing request runs the reconnection logic on its
own. The Connecting state check does not prevent this, because reading and
setting the state are two separate lock acquisitions, and each recovery calls
disconnect() first, which resets the state and tears down whatever another task
just established. So a single outage can fan out into many concurrent dial
attempts. Fortunately, this one is much easier to solve: recovery just needs to
be single-flight, let the first failing request starts it, everyone else waits
for its result.
This is my current design:
<img width="795" height="609" alt="Image"
src="https://github.com/user-attachments/assets/203d9b04-715e-47a4-8c92-63e02a3b0196";
/>
> It would also help to define whether an in-flight request is failed,
retried, or queued while the connection and login are restored.
No queueing. Recovery is single-flight: the first request that fails with a
retryable error spawns one recovery goroutine (disconnect → connect with the
configured backoff → auto-login). Concurrent failing requests join the same
recovery and wait for its done signal, each bounded by its own context; giving
up the wait does not cancel the recovery. Once recovery completes, whether the
original request is replayed depends on whether the server could have executed
it. Currently, here are some conditions we can consider:
- server replied with a session error (Unauthenticated / StaleClient): not
executed, safe to replay after re-login
- request frame was never fully written: not executed, safe to replay
- frame fully written but no response arrived: the server may have executed
it. This one is tricky, we need to figure out how.
Requests that arrive while recovery is still running fail with
not-connected, join the same wait, and are sent after recovery — they were
never sent, so that is always safe. (Or if the caller set context timeout or
canceled it, they will fail)
--
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]
