You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "TomPlanche (via GitHub)" <gi...@apache.org> on 2026/10/01 13:36:04 UTC
[PR] fix(sdk): keep a shut down client shut down [iggy]
TomPlanche opened a new pull request, #4374:
URL: https://github.com/apache/iggy/pull/4374
## Which issue does this PR address?
Relates to #4287
## Rationale
`shutdown()` must be final, as agreed on #4287: a client that is shut down must not connect again, whatever the caller does next. Problems 1 and 4 of #4287 break this rule. This PR fixes these two problems. Problems 2 and 3, about `disconnect()`, come in a separate PR.
## What changed?
A client that was shut down could come back. `WebSocketClient` dialed again on `connect()`, because its `connect_inner` had no `Shutdown` arm. On every transport, a `disconnect()` after `shutdown()` wrote `Disconnected` over `Shutdown`, so `connect()` worked again.
Now one guard in `set_state` keeps `Shutdown` final on TCP, QUIC and WebSocket, so no later write can revive the client. This guard also covers the `Disconnected` writes that other paths add, for example a timeout. `WebSocketClient::connect_inner` also returns `ClientShutdown` before any dial, as TCP and QUIC already do. The guard alone does not stop that dial.
The `Client` trait now documents this contract on `shutdown()`. The two lifecycle tests from #4344 for these cases lose their `#[ignore]`. On the Python side, the strict `xfail` of the WebSocket case and the known-issue note of `shutdown()` go away, because the bug is fixed.
## Local Execution
I used Zed in devcontainer mode. The build and the tests ran in the devcontainer, and `prek` ran on macOS.
- Passed: `cargo fmt --all` and `cargo clippy -p iggy_common -p iggy -p integration --all-features --all-targets -- -D warnings`.
- Passed: `cargo test -p iggy --lib`, with 232 tests.
- Passed: the lifecycle tests. The 4 cases of problems 1 and 4 pass, and the 6 cases of problems 2 and 3 stay ignored:
```text
$ cargo test -p integration -- sdk::client_lifecycle
test result: ok. 4 passed; 0 failed; 6 ignored; 0 measured; 991 filtered out
```
- Passed: the `sdk::` integration suite (63 passed, 6 ignored), in two runs, and the `cluster::` suite (80 passed), because the guard changes state transitions that reconnects and failovers use.
- Passed: the Python lifecycle tests against a local server, with no `xfail` left:
```text
$ uv run --no-sync pytest tests/test_connectivity.py -k Lifecycle
16 passed, 35 deselected
```
- Passed: `cargo run --bin stub_gen`. The stub matches the edited docstring.
- Passed: pre-commit hooks with `SKIP=web-lint prek run --all-files`. I skipped `web-lint` because this PR does not change `web/`.
## AI Usage
1. No
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sdk): keep a shut down client shut down [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #4374:
URL: https://github.com/apache/iggy/pull/4374#discussion_r4161207815
##########
core/sdk/src/websocket/websocket_client.rs:
##########
@@ -600,8 +605,13 @@ impl WebSocketClient {
let settle_off_leader = context.settle_off_leader();
let single_attempt = context.single_attempt();
loop {
- if self.get_state().await == ClientState::Connected {
- return Ok(());
+ match self.get_state().await {
Review Comment:
warning: `shutdown()` does not stop a `connect()` that is already dialing, on TCP, QUIC or WebSocket. The dial and retry loops never re-read the state, so a later dial installs a live stream and `connect()` can return `Ok` on a shut-down client. Re-check `ClientState::Shutdown` after each dial, then close the stream and return `ClientShutdown`.
##########
core/sdk/src/tcp/tcp_client.rs:
##########
@@ -210,7 +210,12 @@ impl BinaryTransport for TcpClient {
}
async fn set_state(&self, state: ClientState) {
- *self.state.lock().await = state;
+ let mut current = self.state.lock().await;
Review Comment:
simplification: The six-line guard is copied into `set_state` on TCP, QUIC and WebSocket. One helper that applies the transition to a `&mut ClientState` under the lock keeps the rule in one place. Saves about 15 lines.
##########
core/sdk/src/quic/quic_client.rs:
##########
@@ -156,7 +156,12 @@ impl BinaryTransport for QuicClient {
}
async fn set_state(&self, state: ClientState) {
- *self.state.lock().await = state;
+ let mut current = self.state.lock().await;
+ // Shutdown is final: a later write, such as a disconnect or a lost
+ // connection, must not make the client usable again.
+ if *current != ClientState::Shutdown {
Review Comment:
nit: After `shutdown()`, `disconnect()` publishes a `DiagnosticEvent::Disconnected` although the state stays `Shutdown`, so subscribers see a state the client cannot reach. Publish the event only when the state write succeeds. Also at `core/sdk/src/tcp/tcp_client.rs:1298` and `core/sdk/src/websocket/websocket_client.rs:1050`.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sdk): keep a shut down client shut down [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio merged PR #4374:
URL: https://github.com/apache/iggy/pull/4374
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sdk): keep a shut down client shut down [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on PR #4374:
URL: https://github.com/apache/iggy/pull/4374#issuecomment-5941928308
/skill team-review-slim
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sdk): keep a shut down client shut down [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4374:
URL: https://github.com/apache/iggy/pull/4374#issuecomment-5932582841
<!-- iggy-pr-triage-welcome -->
Thanks for the PR. It is labeled `S-waiting-on-review` and queued for review.
Slash commands (own line, regular comment) move it around the queue:
- `/ready` - back to `S-waiting-on-review` after addressing feedback
- `/author` - flip to `S-waiting-on-author` while you finish changes
- `/request-review @user-or-team` - request a reviewer
- `/pin` - exempt the PR from the stale bot, `/unpin` to undo
See [CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/CONTRIBUTING.md#pr-triage-commands) for details.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(sdk): keep a shut down client shut down [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #4374:
URL: https://github.com/apache/iggy/pull/4374#issuecomment-5932999554
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/4374?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `80.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 87.85%. Comparing base ([`a08d0ca`](https://app.codecov.io/gh/apache/iggy/commit/a08d0ca8fc5e2917650f3874d769c2ad7df3b900?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1aa5888`](https://app.codecov.io/gh/apache/iggy/commit/1aa5888f41315b999c774dc7bb3e56e9752a8c75?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/4374?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [core/sdk/src/websocket/websocket\_client.rs](https://app.codecov.io/gh/apache/iggy/pull/4374?src=pr&el=tree&filepath=core%2Fsdk%2Fsrc%2Fwebsocket%2Fwebsocket_client.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZGsvc3JjL3dlYnNvY2tldC93ZWJzb2NrZXRfY2xpZW50LnJz) | 80.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4374?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #4374 +/- ##
============================================
- Coverage 87.85% 87.85% -0.01%
Complexity 1577 1577
============================================
Files 1290 1290
Lines 228835 228837 +2
Branches 192195 192197 +2
============================================
- Hits 201051 201034 -17
+ Misses 23062 23045 -17
- Partials 4722 4758 +36
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `88.94% <80.00%> (-0.01%)` | :arrow_down: |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.73% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.44% <ø> (ø)` | |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.24% <ø> (ø)` | |
| [PHP SDK](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.67% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `96.57% <ø> (ø)` | |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/4374/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.30% <ø> (ø)` | |
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/4374?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [core/sdk/src/quic/quic\_client.rs](https://app.codecov.io/gh/apache/iggy/pull/4374?src=pr&el=tree&filepath=core%2Fsdk%2Fsrc%2Fquic%2Fquic_client.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZGsvc3JjL3F1aWMvcXVpY19jbGllbnQucnM=) | `79.14% <ø> (+2.13%)` | :arrow_up: |
| [core/sdk/src/tcp/tcp\_client.rs](https://app.codecov.io/gh/apache/iggy/pull/4374?src=pr&el=tree&filepath=core%2Fsdk%2Fsrc%2Ftcp%2Ftcp_client.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZGsvc3JjL3RjcC90Y3BfY2xpZW50LnJz) | `90.11% <ø> (ø)` | |
| [foreign/python/src/client.rs](https://app.codecov.io/gh/apache/iggy/pull/4374?src=pr&el=tree&filepath=foreign%2Fpython%2Fsrc%2Fclient.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Zm9yZWlnbi9weXRob24vc3JjL2NsaWVudC5ycw==) | `99.77% <ø> (ø)` | |
| [core/sdk/src/websocket/websocket\_client.rs](https://app.codecov.io/gh/apache/iggy/pull/4374?src=pr&el=tree&filepath=core%2Fsdk%2Fsrc%2Fwebsocket%2Fwebsocket_client.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZGsvc3JjL3dlYnNvY2tldC93ZWJzb2NrZXRfY2xpZW50LnJz) | `80.31% <80.00%> (+2.56%)` | :arrow_up: |
... and [50 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/4374/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org