Fix statement/batch request timeouts #341
Merged
Conversation
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
Adds implementation of RequestTimeout as a `MaybeUnsetConfigValue` type, allowing it to be used in the same way as other config values that can be unset.
Request timeout handling in `cass_statement_set_request_timeout` was broken: it was implemented by hand, which meant that it did not cooperate with execution profiles. In particular, if someone set a longer timeout on a statement than the one set in the execution profile, then the execution profile's timeout was still used. This commit fixes this by using the dedicated Rust Driver Statement's API for setting the request timeout, which correctly cooperates with execution profiles (including the cluster's default request timeout).
Request timeout handling in `cass_batch_set_request_timeout` was broken: 1. It was implemented by hand, which meant that it did not cooperate with execution profiles. In particular, if someone set a longer timeout on a statement than the one set in the execution profile, then the execution profile's timeout was still used. 2. It did not handle the `0` value correctly - it was interpreted as a request timeout of 0 milliseconds, while it should be interpreted as "no timeout". This commit fixes both problems.
4 tasks
There was a problem hiding this comment.
Pull Request Overview
This PR replaces hand-crafted per-statement and per-batch timeout handling with the Rust driver’s built-in timeout API, using the MaybeUnsetConfig framework to correctly interpret zero, max, and unset values.
- Introduces a new
RequestTimeouttype and conversion logic inconfig_value.rs - Updates
cass_statement_set_request_timeoutandcass_batch_set_request_timeoutto call the driver’sset_request_timeoutwith the right semantics - Removes manual storage of timeout values and
tokio::timeoutwrappers insession.rs
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scylla-rust-wrapper/src/config_value.rs | Added RequestTimeout and infallible C-value conversion |
| scylla-rust-wrapper/src/statement.rs | Hooked into driver’s set_request_timeout, removed old field |
| scylla-rust-wrapper/src/batch.rs | Same update for batches |
| scylla-rust-wrapper/src/session.rs | Deleted custom timeout futures and related fields |
Comments suppressed due to low confidence (2)
scylla-rust-wrapper/src/config_value.rs:113
- Add unit tests for
RequestTimeoutto verify that C values 0, MAX, and typical values map to the intendedOption<Duration>outcomes.
pub(crate) struct RequestTimeout(pub(crate) Option<Duration>);
scylla-rust-wrapper/src/config_value.rs:112
- [nitpick] Consider adding a doc comment on
RequestTimeoutto explain its semantics (how zero → infinite, MAX → unset, and other values).
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
6 tasks
Lorak-mmk
approved these changes
Jul 8, 2025
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.
Problem statement
The implementation of request timeouts set on statement/batch is hand-crafted and independent from the Rust Driver-provided request timeouts set on cluster/execution profile. This is obviously incorrect, as a longer request timeout on statement/batch would not override a shorter timeout set on cluster/execution profile.
Solution
This PR leverages the
MaybeUnsetConfigframework to work with request timeouts, and fixes statements and batches to use the Rust Driver's built-in request timeout capabilities.Care is taken to support the semantics correctly:
timeout = 0means that there should be no timeout. As the Rust driver does not allow for such setting on the statement/batch level, it is emulated using an extremely long timeout.timeout = u64::MAXmeans that the timeout should be unset (and the execution profile's/cluster's setting should be used instead). This corresponds to passingNoneto the Rust Driver's{Statement,Batch}::set_request_timeout.Pre-review checklist
[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.