Rust SDK: disconnect() returns before a reconnect in flight stops #4287

Edit: added problem 4, found by the automated review of #4288.
Edit 2: added problems 5 to 8, found by @hubcio in the review of #4288.
Edit 3: added the Status section after #4344, #4374 and #4388 were merged.
Edit 4: updated the Status section after #4501, #4506 and #4507 were merged.

Status

The tests from #4344 pin problems 1 to 4 in core/integration/tests/sdk/client_lifecycle.rs. All 10 cases run on master, and none of them is ignored.

• Problem 1: fixed in fix(sdk): keep a shut down client shut down #4374. WebSocketClient::connect_inner returns ClientShutdown after shutdown(), as TCP and QUIC do.
• Problem 2: fixed in fix(sdk): keep an explicit disconnect until the next connect #4388. disconnect() sets a caller-intent flag that holds until the next connect(), and IggyClient::disconnect() stops the heartbeat. The Rust, Python and C++ docs describe the new contract.
• Problem 3: fixed in fix(sdk): keep an explicit disconnect until the next connect #4388. As agreed with @hubcio, ping() and get_stats() after disconnect() fail with NotConnected on the SDK side, with no packets on the wire and no reconnect.
• Problem 4: fixed in fix(sdk): keep a shut down client shut down #4374. A guard in set_state keeps Shutdown final on TCP, QUIC and WebSocket.
• Problem 5: fixed in fix(sdk): stop a reconnect sweep when the client shuts down #4507. A reconnect sweep stops with ClientShutdown before each dial and right after it installs a connection, on TCP, QUIC and WebSocket. shutdown() sets Shutdown before it takes the connection, and a call after a cancelled shutdown() finishes the cleanup.
• Problem 6: partly fixed. Since fix(sdk): keep an explicit disconnect until the next connect #4388, the reconnect sweep checks the flag before each dial, after each dial and before each retry pause, on TCP, QUIC and WebSocket, so a sweep in flight ends with NotConnected. But disconnect() still returns before that sweep ends. On TCP with a 3 s retry interval, the state stays Connecting for 2.8 s after disconnect() returns, and a connect() after the disconnect() waits for that pause too.
• Problem 7: fixed in feat(server)!: route partition requests to their primaries #4506. TcpClient::shutdown closes the link with link.shutdown(), which returns no error, so a failed close can no longer skip set_state(ClientState::Shutdown). Since fix(sdk): stop a reconnect sweep when the client shuts down #4507, shutdown() also sets Shutdown before it closes the link.
• Problem 8: partly fixed. fix(sdk): keep an explicit disconnect until the next connect #4388 stops the heartbeat in disconnect() and blocks redials after it, so the heartbeat case of client_lifecycle.rs no longer hangs. The QUIC disconnect() still waits in endpoint.wait_idle(), and a sweep can still dial during that wait.

Related: #4327 (the clients block after 1000 diagnostic events, fixed in #4501) and #4345 (bounded TCP login and connect, in the same reconnect loops as problem 6).

Bug description

This issue lists eight problems in the lifecycle of the Rust IggyClient. I found problems 1 to 4 while I worked on #4163. They occur on master at commit 5d8129e95, without local changes, and a program reproduces each of them. @hubcio found problems 5 to 8 in the review of #4288. I checked them against the code on master at commit 9aff4424a. They are races between tasks, thus no program reproduces them yet.

1. A WebSocket client can connect again after shutdown()

After shutdown(), TCP and QUIC reject connect() with ClientShutdown. WebSocketClient does not check the Shutdown state in connect_inner. It only returns early in the Connected state. Thus connect() opens a new connection on a client that is shut down.

• Expected: connect() after shutdown() returns Err(ClientShutdown) on every transport.
• Actual: TCP returns Err(ClientShutdown), WebSocket returns Ok(()).

2. With auto-login, the heartbeat undoes an explicit disconnect()

IggyClient::disconnect() does not stop the heartbeat task that connect() starts. The heartbeat sends a ping() at each interval (5 s by default). If the client has auto-login credentials, this ping connects the client again and replays the auto-login. As a result, an explicit disconnect() stops working after one heartbeat interval, without an error.

• Expected: after disconnect(), requests fail until the caller calls connect().
• Actual: get_me() fails right after disconnect(), then succeeds 7 s later.

The documentation and the tests do not agree on the correct behavior:

• The IggyClient documentation describes this behavior: "if auto-login is configured, the client will reconnect automatically and undo the disconnect".
• The test disconnect_relogin.rs states the opposite contract: "an operation after an explicit disconnect must fail on the dead transport instead of reconnecting into a resurrected session". This test covers only a client that signed in with login_user(), not an auto-login client.

3. After disconnect(), a direct ping() reconnects but an authenticated request does not

For the same auto-login client, the result of a request after disconnect() depends on the request type:

• get_me() fails with Disconnected. fail_if_not_authenticated rejects the request before the transport can reconnect.
• ping() does not go through this check. send_raw_with_response gets NotConnected, finds the configured credentials, and reconnects. After this ping(), get_me() succeeds.

I do not know which behavior is correct here. Must a direct ping() after an explicit disconnect() reconnect an auto-login client?

4. disconnect() after shutdown() makes the client reusable

No transport checks the Shutdown state in disconnect(). TcpClient::disconnect_transport returns early only for Disconnected and Connecting. QuicClient::disconnect and WebSocketClient::disconnect return early only for Disconnected. Thus disconnect() changes the state from Shutdown to Disconnected, and connect() works again.

• Expected: after shutdown(), disconnect() keeps the client shut down, and connect() returns Err(ClientShutdown).
• Actual: connect() returns Ok(()) on TCP, WebSocket and QUIC.

5. A connect in flight during shutdown() brings the client back

shutdown() does not stop a connect that runs at the same time, for example a reconnect. When the dial succeeds, TcpClient::connect_inner sets the state to Connected without a check for Shutdown, then signs in again with the auto-login credentials. QUIC and WebSocket do the same.

• Expected: a connect in flight stops when shutdown() runs, and the client stays shut down.
• Actual: the client is Connected again after shutdown() returns.

6. disconnect() returns Ok while a connect is in flight

In the Connecting state, TcpClient::disconnect_transport returns Ok(()) and does nothing. The connect in flight then completes, and the client is connected again, also without auto-login. Problems 5 and 6 have the same cause: the connect does not check the state that shutdown() or disconnect() sets in the meantime.

• Expected: after disconnect() returns, the client is disconnected.
• Actual: the client is connected when the connect in flight completes.

7. A failed close in shutdown() leaves the client usable

TcpClient::shutdown closes the stream with stream.shutdown().await?. If the close fails, for example over TLS after the peer resets the connection, the ? returns before set_state(ClientState::Shutdown). The state stays connected without a stream, and the heartbeat connects the client again. WebSocketClient::shutdown ignores the close error and always sets Shutdown.

• Expected: shutdown() sets Shutdown even when the close fails.
• Actual: the client is not shut down, and it connects again.

8. A redial during disconnect() blocks QUIC disconnect()

QuicClient::disconnect calls endpoint.wait_idle(), which waits until every connection on the endpoint closes. If a redial opens a new connection on the same endpoint during this wait, disconnect() stays blocked for as long as the new connection lives.

• Expected: disconnect() returns when the connection that it closes is closed.
• Actual: disconnect() waits for the new connection too.

Possible fixes

This is the plan from before the fixes. The Status section above shows what is done.

I have local fixes for problems 1 and 2, with tests. Each test fails without its fix:

• Problem 1: connect_inner in WebSocketClient returns ClientShutdown in the Shutdown state, as TCP and QUIC do. A unit test checks connect() after shutdown().
• Problem 2: IggyClient::disconnect() stops the heartbeat task, and connect() starts it again. An integration test in disconnect_relogin.rs uses an auto-login client with a 100 ms heartbeat and checks that the client stays disconnected. This fix changes documented behavior, thus the documentation of IggyClient must change too.

For problem 4, disconnect() on each transport can return early in the Shutdown state, as shutdown() already does. I have no local fix for problem 4 yet.

For problems 5 and 6, the connect can check the state again before it installs the connection, and give up when shutdown() or disconnect() ran in the meantime. For problem 7, shutdown() can set Shutdown before it closes the stream, as WebSocket already does. I have no local fix for problems 5 to 8 yet.

The fix for problem 2 does not change problem 3. I can open one pull request for each fix when a maintainer confirms the correct behavior for problems 2 and 3.

Affected area / component

Rust SDK

Deployment

Compiled from source

Versions

Iggy server and Rust SDK: master at commit 5d8129e95de1d1fdf2ed42af015340b82af1c59a

Hardware / environment

The server runs on Linux in a devcontainer (aarch64). The reproduction programs run on macOS and connect to the ports that the container publishes.

Sample code

The programs are in repros/sdk-lifecycle on my fork. The crate uses iggy from apache/iggy at commit 5d8129e95. The shortest program shows problem 2:

use iggy::prelude::*;
use std::time::Duration;

#[tokio::main]
async fn main() {
    let client = IggyClient::from_connection_string("iggy+tcp://iggy:[email protected]:8090").unwrap();
    client.connect().await.unwrap();
    client.disconnect().await.unwrap();
    println!("right after disconnect: get_me -> {:?}", client.get_me().await.map(|_| ()));

    tokio::time::sleep(Duration::from_secs(7)).await;
    println!("7 s after disconnect:   get_me -> {:?}", client.get_me().await.map(|_| ()));
}

Logs

$ cargo run --bin ws_connect_after_shutdown
iggy+tcp://iggy:[email protected]:8090: connect after shutdown -> Err(ClientShutdown)
iggy+ws://iggy:[email protected]:8092: connect after shutdown -> Ok(())

$ cargo run --bin heartbeat_undoes_disconnect
right after disconnect: get_me -> Err(Disconnected)
7 s after disconnect:   get_me -> Ok(())

$ cargo run --bin ping_reconnects_after_disconnect
get_me -> Err(Disconnected)
ping   -> Ok(())
get_me -> Ok(())

$ cargo run --bin disconnect_after_shutdown
iggy+tcp://iggy:[email protected]:8090: connect after shutdown + disconnect -> Ok(())
iggy+ws://iggy:[email protected]:8092: connect after shutdown + disconnect -> Ok(())
iggy+quic://iggy:[email protected]:8080: connect after shutdown + disconnect -> Ok(())

Iggy server config

No response

Reproduction

1. From the root of an apache/iggy checkout on Linux, start a server with the default root credentials:

   cargo run --bin iggy-server -- --with-default-root-credentials --fresh

2. Clone the tomplanche/repros branch of my fork, then go to repros/sdk-lifecycle.

3. Run cargo run --bin ws_connect_after_shutdown. The WebSocket line shows Ok(()).

4. Run cargo run --bin heartbeat_undoes_disconnect. The second get_me shows Ok(()).

5. Run cargo run --bin ping_reconnects_after_disconnect. The ping and the second get_me show Ok(()).

6. Run cargo run --bin disconnect_after_shutdown. The three lines show Ok(()).

Contribution

• I'm willing to submit a pull request to fix this bug

Good first issue

• I think this could be a good first issue for a new contributor