Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented client-side per-session and per-statement configurable timeouts #522

Merged
merged 5 commits into from
Aug 23, 2022

Conversation

wprzytula
Copy link
Collaborator

@wprzytula wprzytula commented Aug 19, 2022

Two levels of client-side timeouts were added: per Session and per Statement (PreparedStatement and Query). If no timeout is specified for a statement, a global per-session one is used. Once a timeout elapses, an RequestTimeout error is returned.

Fixes: #304

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I added appropriate Fixes: annotations to PR description.

@wprzytula
Copy link
Collaborator Author

@psarna @cvybhu Maybe you could help me with understanding why test_client_timeout fails in nondeterministic way.

@cvybhu
Copy link
Contributor

cvybhu commented Aug 19, 2022

@psarna @cvybhu Maybe you could help me with understanding why test_client_timeout fails in nondeterministic way.

tokio::Timeout checks if the future has completed before checking if the timeout has elapsed.
With a local node and fast network it's possible that the query completes in a single Poll maybe?

https://github.com/tokio-rs/tokio/blob/de81985762a242c77361a6ab9de198372ca85987/tokio/src/time/timeout.rs#L175

@cvybhu
Copy link
Contributor

cvybhu commented Aug 19, 2022

Or maybe not, I added tokio::task::yield_now().await; at the beginning of runner block and it still fails.
So there are at least two Polls of the timeout future and it doesn't fail 0_o

@cvybhu
Copy link
Contributor

cvybhu commented Aug 19, 2022

Yeah something like this fails as well:

async fn yields_once() {
    tokio::task::yield_now().await;
}

async fn instant_timeout_test() {
    let res = tokio::time::timeout(std::time::Duration::from_secs(0), yields_once()).await;
    assert!(res.is_err());
}

Although sleeping for 1ms is enough to trigger timeout.

This might be an issue with tokio, but I'm not sure if it's a bug or a feature.
Tokio time keeps mentioning that its precision can be > 1ms, for example for sleep:

The implementation is platform specific, and some platforms (specifically Windows) will provide timers with a larger resolution than 1 ms.

https://docs.rs/tokio/latest/tokio/time/fn.sleep.html

@cvybhu
Copy link
Contributor

cvybhu commented Aug 19, 2022

Tokio time keeps mentioning that its precision can be > 1ms, for example for sleep:

I think we can just assume that tokio doesn't support timeouts of < 1ms and wait with the tests until proxy is merged.

@wprzytula
Copy link
Collaborator Author

Tokio time keeps mentioning that its precision can be > 1ms, for example for sleep:

I think we can just assume that tokio doesn't support timeouts of < 1ms and wait with the tests until proxy is merged.

That's right, with proxy we will easily delay the response for a couple of millis and set the timeout for 1 ms - this should work.

scylla-cql/src/errors.rs Outdated Show resolved Hide resolved
@psarna
Copy link
Contributor

psarna commented Aug 19, 2022

@wprzytula for now, let's just verify manually. If you create a 0ms or 1ms timeout, it should properly time out when sent to Scylla booted on a remote machine (e.g. one of the office servers).

@wprzytula wprzytula force-pushed the timeouts branch 3 times, most recently from 2b7e244 to 1466e6d Compare August 22, 2022 08:39
This shall represent the situation when the timeout provided
elapsed before the query was completed.
@wprzytula wprzytula marked this pull request as ready for review August 22, 2022 09:46
@wprzytula wprzytula requested a review from psarna August 22, 2022 09:46
@wprzytula
Copy link
Collaborator Author

v2:

  • client_timeout -> request_timeout
  • timeout test made working with 1ms timeout and remote Scylla; test marked as ignored (for normal testing with local Scylla).

scylla-cql/src/errors.rs Show resolved Hide resolved
scylla/src/statement/mod.rs Outdated Show resolved Hide resolved
scylla/src/transport/session_test.rs Outdated Show resolved Hide resolved
scylla/src/transport/session_test.rs Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Show resolved Hide resolved
StatementConfig had this field added, and PreparedStatement and Query
got getters and setters for that.
Session, SessionConfig and SessionBuilder were equipped with such field.
The whole run_query() was made a one big async block, which is now
polled together with a timeout using tokio timeout. Once a timeout
elapses, the query is cancelled and QueryError::RequestTimeout
is returned.
As tokio timeout supports minimal granularity of 1 ms, the test can
be only reliable on remote Scyllas, as local ones respond faster.
Therefore, the rest is marked as ignored.
As all ignored tests were run in Authenticate CI workflow, it was made
specific to avoid running the new request timeouts test.
@wprzytula
Copy link
Collaborator Author

@psarna This is probably complete.

@psarna psarna merged commit c449270 into scylladb:main Aug 23, 2022
@wprzytula wprzytula deleted the timeouts branch August 23, 2022 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement client-side timeouts
4 participants