Repository navigation
Conversation
Author
|
@kezhuw any thoughts on this? something you'd be willing to land? |
Author
|
@kezhuw bumping here, wanted to know your thoughts on this |
kezhuw
reviewed
Oct 20, 2026
kezhuw
left a comment
kezhuw
left a comment
Owner
There was a problem hiding this comment.
Thank you! Looks good to me. I left some inline comments about the details.
| tokio = ["asyncs/tokio"] | ||
| smol = ["asyncs/smol"] | ||
| async-global-executor = ["asyncs/async-global-executor"] | ||
| turmoil = ["dep:turmoil", "dep:tokio-util", "tokio"] |
Owner
There was a problem hiding this comment.
The test fails due to unused async_net in case of turmoil. Does rust have way to opt out dependency in case of feature ?
If not, we probably have to use it in net module to circumvent this. I think it is ok for turmoil support.
| //! * `sasl`: Toggle SASL support. | ||
| //! * `sasl-gssapi`: Toggle only GSSAPI SASL support. This relies on binding package `libgssapi-sys`. | ||
| //! * `sasl-digest-md5`: Toggle only DIGEST-MD5 SASL support. | ||
| //! * `turmoil`: Toggle simulated networking support via `turmoil`. |
Owner
There was a problem hiding this comment.
How about move this to a separated section "Network simulation" for turmoil ?
| //! * `async-global-executor`: Toggle support for [async-global-executor](https://docs.rs/async-global-executor). | ||
| //! | ||
| //! `turmoil` currently builds on the `tokio` runtime support and adapts simulated sockets into the | ||
| //! crate's existing `futures::io` transport layer. |
Owner
There was a problem hiding this comment.
adapts simulated sockets into the crate's existing
futures::iotransport layer
Is this an implmentation detail ?
We should point out that Client must be constructed on tokio tasks for turmoil to function.
CI fails on `fmt` and `build`, and `coverage` would fail next: - build: with `turmoil`, `async-net` goes unused and `-Dunused-crate-dependencies` rejects it. Cargo features are additive, so the feature can't drop the non-optional dependency it replaces; mark it used in `net` instead. - fmt: split a test import as nightly rustfmt wants. - coverage: `--all-features` turns on `turmoil`, under which the client connects only inside a turmoil simulation, so the real ZooKeeper and TLS tests panic. Run them with every feature but `turmoil`, and run the turmoil tests in their own step so they still run in CI. The feature matrix runs the whole suite per feature set, so `turmoil` can't join it without gating every real-socket test. `make test` follows suit. Move the `turmoil` docs to their own "Network simulation" section and replace the implementation detail with what callers must do: construct `Client` on tokio tasks of a turmoil host, and seed fastrand, which turmoil's seed doesn't reach, for reproducible endpoint shuffle and reconnect jitter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The turmoil feature moves sockets, TLS and DNS into the simulation, but every timer and the clock stay on async-io and std wall time. Turmoil advances simulated time far faster than wall time, so connect timeouts, reconnect backoff, pings and session expiry fire at the wrong simulated moment, or not at all within the simulated duration, and the client's view of elapsed time drifts from the simulation's. That defeats simulating retries and session behavior. Add a `time` module next to `net` with `Timer` and `now`. Under `turmoil` they run on tokio's clock, which turmoil advances: `Timer` mirrors the never/once/periodic `async_io::Timer` API the call sites use, including the first periodic tick being one period out, and `now` converts tokio's `Instant` back to `std`. Otherwise they are `async_io::Timer` and `Instant::now`, so default builds are unchanged. Switch the session, connection, endpoint and deadline call sites to it. `async-io` then goes unused under `turmoil`, so mark it used like `async-net`. Under `turmoil` the session loops' `select!` is `biased`: an unbiased `select!` picks the branch to poll first from the address of its futures, which differs between runs, so the same seed could take different branches. The connect `select!` is already biased. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
@kezhuw thanks for taking a look, addressing your comments. going to refresh this a bit, there are the timer apis we also need to abstract for turmoil + also going to rebase off latest. |
jttdd
force-pushed
the
turmoil-support
branch
2 times, most recently
from
October 21, 2026 14:46
2f5173a to
6937882
Compare
The session fails pending requests, notifies watchers of session state and resends watches on reconnect by walking hashbrown maps. hashbrown's default hasher (foldhash) seeds each map from addresses and other per-run state that a simulation's seed doesn't reach, so after a disconnect the same seed could wake callers and watchers in a different order, and the run diverge from there. Under `turmoil` the session's maps hash with std's fixed-key SipHash, so the same inserts traverse in the same order on every run. Otherwise they keep hashbrown's default hasher, so default builds are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this PR adds optional turmoil socket support to be able to run the client within a turmoil simulation.
I’d like to use this client in deterministic simulation tests, where I can exercise connection loss, partitions, delayed traffic, retries, and session behavior without relying on timing-sensitive real sockets or external ZooKeeper infrastructure. Turmoil is a good fit for that kind of coverage, and this change keeps the support small and opt-in while preserving the crate’s existing async-runtime-agnostic shape.