TomPlanche opened a new issue, #4287: URL: https://github.com/apache/iggy/issues/4287
### Bug description ### Bug description I found three problems in the lifecycle of the Rust `IggyClient` while I worked on #4163. All three occur on master at commit `5d8129e95`, without local changes. #### 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`](https://github.com/apache/iggy/blob/5d8129e95de1d1fdf2ed42af015340b82af1c59a/core/sdk/src/websocket/websocket_client.rs#L599-L605). 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()`](https://github.com/apache/iggy/blob/5d8129e95de1d1fdf2ed42af015340b82af1c59a/core/sdk/src/clients/client.rs#L996-L998) 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](https://github.com/apache/iggy/blob/5d8129e95de1d1fdf2ed42af015340b82af1c59a/core/sdk/src/clients/client.rs#L124-L126) describes this behavior: "if `auto-login` is configured, the client will reconnect automatically and undo the disconnect". - The test [`disconnect_relogin.rs`](https://github.com/apache/iggy/blob/5d8129e95de1d1fdf2ed42af015340b82af1c59a/core/integration/tests/sdk/disconnect_relogin.rs#L51-L52) 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`](https://github.com/apache/iggy/blob/5d8129e95de1d1fdf2ed42af015340b82af1c59a/core/common/src/traits/binary_auth.rs#L20-L29) rejects the request before the transport can reconnect. - `ping()` does not go through this check. [`send_raw_with_response`](https://github.com/apache/iggy/blob/5d8129e95de1d1fdf2ed42af015340b82af1c59a/core/sdk/src/tcp/tcp_client.rs#L222-L265) 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? ### Possible fixes 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. 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`](https://github.com/TomPlanche/iggy/tree/tomplanche/repros/repros/sdk-lifecycle) on my fork. The crate uses `iggy` from `apache/iggy` at commit `5d8129e95`. The shortest program shows problem 2: ```rust 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 ```text $ 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(()) ``` ### 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: ```bash 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(())`. ### Contribution - [x] I'm willing to submit a pull request to fix this bug ### Good first issue - [x] I think this could be a good first issue for a new contributor -- 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]
