future: cass future wait timed #147

muzarski · 2024-07-31T07:06:25Z

ref: #42

This PR implements a cass_future_wait_timed API function which allows to wait for a future to resolve within a given period of time.

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
PR description sums up the changes and reasons why they should be introduced.
I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

scylla-rust-wrapper/src/future.rs

muzarski · 2024-08-01T14:17:52Z

Rebased on master.

muzarski · 2024-08-01T15:48:12Z

The async test I enabled started to fail (even when I only rebased on master). I can't reproduce it locally:

for scylla tests, my local version of ccm for some reason can't create a 5.0.0 scylla cluster
for cassandra - the test passes

I'll update an integration test to make use of cass_future_wait instead of cass_future_wait_timed to see if the issue is with the timed version itself, or not.

muzarski · 2024-08-01T16:15:02Z

I changed wait_timed to wait in integration test. There are multiple client timeout errors:

 /home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/objects/future.hpp:152: Failure
      Expected: CASS_OK
To be equal to: wait_code
      Which is: CASS_ERROR_LIB_REQUEST_TIMED_OUT [Request timed out]
Request timed out: Request timeout: Request took longer than 12000ms: deadline has elapsed

Link to the job: https://github.com/scylladb/cpp-rust-driver/actions/runs/10201719963/job/28224244570?pr=153#step:7:8477.

Recently, we merged a PR that sets a default client timeout to 12000ms (which is huge!). I'm not sure why the timeouts appear - these are simple insert queries. Link to the test case: https://github.com/scylladb/cpp-rust-driver/blob/master/tests/src/integration/tests/test_async.cpp#L99-L111.

I'll try to investigate further, but it seems that the test failure is not related to cass_future_wait_timed. The test fails due to client request timeouts. As I mentioned previously, the test passes for me locally. Maybe it's some GH actions issue - query timing out after 12s is really weird.

cc: @Lorak-mmk @wprzytula

Lorak-mmk · 2024-08-02T21:09:38Z

The async test I enabled started to fail (even when I only rebased on master). I can't reproduce it locally:
* for scylla tests, my local version of ccm for some reason can't create a 5.0.0 scylla cluster

What is the error? also we should update to newer version / test with more than 1 version.

* for cassandra - the test passes
I'll update an integration test to make use of cass_future_wait instead of cass_future_wait_timed to see if the issue is with the timed version itself, or not.

Lorak-mmk · 2024-08-02T21:11:03Z

I changed wait_timed to wait in integration test. There are multiple client timeout errors:
 /home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/objects/future.hpp:152: Failure
      Expected: CASS_OK
To be equal to: wait_code
      Which is: CASS_ERROR_LIB_REQUEST_TIMED_OUT [Request timed out]
Request timed out: Request timeout: Request took longer than 12000ms: deadline has elapsed
Link to the job: https://github.com/scylladb/cpp-rust-driver/actions/runs/10201719963/job/28224244570?pr=153#step:7:8477.

Recently, we merged a PR that sets a default client timeout to 12000ms (which is huge!). I'm not sure why the timeouts appear - these are simple insert queries. Link to the test case: https://github.com/scylladb/cpp-rust-driver/blob/master/tests/src/integration/tests/test_async.cpp#L99-L111.

I'll try to investigate further, but it seems that the test failure is not related to cass_future_wait_timed. The test fails due to client request timeouts. As I mentioned previously, the test passes for me locally. Maybe it's some GH actions issue - query timing out after 12s is really weird.

cc: @Lorak-mmk @wprzytula

So previously there was no client request timeout at all? And now that you introduced it, queries are timing out?
Tests are running in valgrind, so it's possible for queries to take a longer time, but it shouldn't be this long.
When running locally, do you also use valgrind?

scylla-rust-wrapper/src/future.rs

muzarski · 2024-08-04T20:08:14Z

So previously there was no client request timeout at all? And now that you introduced it, queries are timing out? Tests are running in valgrind, so it's possible for queries to take a longer time, but it shouldn't be this long. When running locally, do you also use valgrind?

Good thing that you noticed that. I missed it. Indeed, when I run the tests locally under valgrind, the timeouts appear.

muzarski · 2024-08-04T20:21:06Z

The async test I enabled started to fail (even when I only rebased on master). I can't reproduce it locally:
* for scylla tests, my local version of ccm for some reason can't create a 5.0.0 scylla cluster
What is the error?

Nvm, it's not ccm issue. I was just missing openjdk-11 locally which is needed to setup 5.0.0 cluster apparently.

also we should update to newer version / test with more than 1 version.

What version would you suggest? 5.4.8?

Lorak-mmk · 2024-08-04T23:16:52Z

The async test I enabled started to fail (even when I only rebased on master). I can't reproduce it locally:
* for scylla tests, my local version of ccm for some reason can't create a 5.0.0 scylla cluster
What is the error?
Nvm, it's not ccm issue. I was just missing openjdk-11 locally which is needed to setup 5.0.0 cluster apparently.

I recommend using ccm by nix, it removed a lot of my frustrations with ccm.

also we should update to newer version / test with more than 1 version.

What version would you suggest? 5.4.8?

6.0

muzarski · 2024-08-05T06:51:37Z

v2: addressed review comments

There are still timeouts appearing. The conclusion is that the timeouts appear due to tests being run under valgrind (reproducing locally as well). I see two solutions here:

Run the timeout-sensitive tests in a separate CI step without valgrind (right now, there is only one failing test due to timeouts).
Modify the failing test so it disables the global client timeout, and waits for the future indefinitely (change wait_timed to wait).

I think the former approach is better, as it allows us to test cass_future_wait_timed. WDYT? @Lorak-mmk @wprzytula

Lorak-mmk · 2024-08-05T07:47:06Z

v2: addressed review comments

There are still timeouts appearing. The conclusion is that the timeouts appear due to tests being run under valgrind (reproducing locally as well). I see two solutions here:
* Run the timeout-sensitive tests in a separate CI step without `valgrind` (right now, there is only one failing test due to timeouts).

* Modify the failing test so it disables the global client timeout, and waits for the future indefinitely (change `wait_timed` to `wait`).
I think the former approach is better, as it allows us to test cass_future_wait_timed. WDYT? @Lorak-mmk @wprzytula

In second approach, if you use wait instead of wait_timed, then how does it test wait_timed?

muzarski · 2024-08-05T07:48:00Z

v2: addressed review comments
There are still timeouts appearing. The conclusion is that the timeouts appear due to tests being run under valgrind (reproducing locally as well). I see two solutions here:
* Run the timeout-sensitive tests in a separate CI step without `valgrind` (right now, there is only one failing test due to timeouts).

* Modify the failing test so it disables the global client timeout, and waits for the future indefinitely (change `wait_timed` to `wait`).
I think the former approach is better, as it allows us to test cass_future_wait_timed. WDYT? @Lorak-mmk @wprzytula
In second approach, if you use wait instead of wait_timed, then how does it test wait_timed?

That's the point - it does not. That's why I prefer to run this specific test without valgrind.

Lorak-mmk · 2024-08-05T07:50:59Z

Sorry, I misread your message.
Separate step complicates the CI, but in this case it seems reasonable to me.

muzarski · 2024-08-05T07:53:21Z

v3: running the failing async test in a separate CI step without valgrind

I'll update Scylla version in CI in a separate PR.

muzarski · 2024-08-05T07:59:43Z

v3.1: deduplicate error message variable in callback test

muzarski · 2024-08-05T08:06:36Z

v3.2: fix CI step description for timeout-sensitive cassandra integration tests

muzarski · 2024-08-07T06:48:34Z

Rebased on master

muzarski · 2024-08-07T07:10:33Z

v4: added proper synchronization between cass_future_wait and cass_future_wait_timed callers.

scylla-rust-wrapper/src/future.rs

muzarski · 2024-09-30T14:01:50Z

v4.1:

rebased on master -> adjusted to new CI
addressed @wprzytula comments

Lorak-mmk · 2024-10-01T11:18:44Z

README.md

Comment regarding commit future: cass_future_wait_timed :
It says Next commit will fix that,, but the next commit is cargo: add futures 0.3 dependency. The commit after that is the one that fixes the problem.

pub(self) is equivalent to not providing a visibility specifier at all.

Note: this implementation does not work for `current-thread` tokio runtime in some cases, causing a deadlock. Two following commits will introduces changes which fix that. We will introduce a synchronization between `cass_future_wait` and `cass_future_wait_timed` callers.

Previously, there was a scenario when using `current-thread` tokio runtime, that caused the deadlock. The scenario described: ``` - we make use of a `current-thread` tokio runtime (`Builder::new_current_thread().enable_all().build()`) - we create a future which sleeps, let's say, 2s and then sets the value - we call `cass_future_wait_timed` on this future, with a timeout 1s. This consumes the handle, starts the future, times out after 1s (there is still a task to be polled until completion - the future from above which sets the value) - now we call `cass_future_wait` on the same future. This waits on cond variable (blocking operation). Now current thread is blocked, it is waiting (on a cond variable) for a value to be set. But there is noone to poll the future that sets the value, since we are using a `current-thread` runtime - deadlock... ``` This commit fixes that, preventing such scenario from happening. Co-authored-by: Karol Baryła <[email protected]>

This test needs to be run without valgrind, as it causes the client request timeouts to appear.

muzarski · 2024-10-01T11:47:46Z

v4.2:

rebased on master
adjusted commit message that @Lorak-mmk mentioned

muzarski requested review from Lorak-mmk and wprzytula July 31, 2024 07:07

muzarski force-pushed the cass_future_wait_timed branch from cae738d to b39f13e Compare July 31, 2024 07:10

muzarski self-assigned this Jul 31, 2024

muzarski force-pushed the cass_future_wait_timed branch from b39f13e to 8a365a4 Compare July 31, 2024 07:35

wprzytula reviewed Jul 31, 2024

View reviewed changes

scylla-rust-wrapper/src/future.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/future.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/future.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/future.rs Outdated Show resolved Hide resolved

muzarski force-pushed the cass_future_wait_timed branch from 8a365a4 to 06bd6b4 Compare August 1, 2024 14:17

muzarski force-pushed the cass_future_wait_timed branch from 06bd6b4 to f188cd3 Compare August 1, 2024 15:07

Lorak-mmk requested changes Aug 2, 2024

View reviewed changes

scylla-rust-wrapper/src/future.rs Outdated Show resolved Hide resolved

scylla-rust-wrapper/src/future.rs Outdated Show resolved Hide resolved

muzarski requested review from wprzytula and Lorak-mmk August 4, 2024 20:26

muzarski force-pushed the cass_future_wait_timed branch from 8b01a95 to 9e8978d Compare August 5, 2024 06:44

muzarski force-pushed the cass_future_wait_timed branch from 9e8978d to 5cc1ba3 Compare August 5, 2024 07:34

muzarski force-pushed the cass_future_wait_timed branch from 5cc1ba3 to 2edd845 Compare August 5, 2024 07:59

muzarski force-pushed the cass_future_wait_timed branch from 2edd845 to 9fcd2d8 Compare August 5, 2024 08:06

muzarski force-pushed the cass_future_wait_timed branch 4 times, most recently from 383d574 to 0202780 Compare August 7, 2024 06:48

muzarski force-pushed the cass_future_wait_timed branch from 0202780 to 869da67 Compare August 7, 2024 07:09

muzarski force-pushed the cass_future_wait_timed branch 3 times, most recently from 053c2b3 to c58ff91 Compare August 7, 2024 07:17

muzarski requested a review from wprzytula August 7, 2024 07:37

wprzytula requested changes Aug 7, 2024

View reviewed changes

muzarski force-pushed the cass_future_wait_timed branch from c58ff91 to bb649a9 Compare September 30, 2024 14:01

muzarski requested a review from wprzytula September 30, 2024 14:23

Lorak-mmk requested changes Oct 1, 2024

View reviewed changes

wprzytula approved these changes Oct 1, 2024

View reviewed changes

muzarski and others added 7 commits October 1, 2024 13:46

future: pub(self) -> no specifier

b1d2743

pub(self) is equivalent to not providing a visibility specifier at all.

testing: utility macro for future's error message

994d6f1

cargo: add futures 0.3 dependency

8504662

future: add tests for callbacks

1d4e2f5

ci: enable an async test using cass_future_wait_timed

e0beb15

This test needs to be run without valgrind, as it causes the client request timeouts to appear.

muzarski force-pushed the cass_future_wait_timed branch from bb649a9 to e0beb15 Compare October 1, 2024 11:47

muzarski requested a review from Lorak-mmk October 1, 2024 11:47

Lorak-mmk approved these changes Oct 1, 2024

View reviewed changes

muzarski merged commit 9ac038f into scylladb:master Oct 1, 2024
7 checks passed

muzarski mentioned this pull request Oct 3, 2024

bump version to 0.2.0 #184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

future: cass future wait timed #147

future: cass future wait timed #147

muzarski commented Jul 31, 2024

muzarski commented Aug 1, 2024

muzarski commented Aug 1, 2024

muzarski commented Aug 1, 2024 •

edited

Loading

Lorak-mmk commented Aug 2, 2024

Lorak-mmk commented Aug 2, 2024

muzarski commented Aug 4, 2024

muzarski commented Aug 4, 2024

Lorak-mmk commented Aug 4, 2024

muzarski commented Aug 5, 2024

Lorak-mmk commented Aug 5, 2024 •

edited

Loading

muzarski commented Aug 5, 2024

Lorak-mmk commented Aug 5, 2024

muzarski commented Aug 5, 2024

muzarski commented Aug 5, 2024

muzarski commented Aug 5, 2024

muzarski commented Aug 7, 2024

muzarski commented Aug 7, 2024

muzarski commented Sep 30, 2024

Lorak-mmk Oct 1, 2024

muzarski commented Oct 1, 2024

future: cass future wait timed #147

future: cass future wait timed #147

Conversation

muzarski commented Jul 31, 2024

Pre-review checklist

muzarski commented Aug 1, 2024

muzarski commented Aug 1, 2024

muzarski commented Aug 1, 2024 • edited Loading

Lorak-mmk commented Aug 2, 2024

Lorak-mmk commented Aug 2, 2024

muzarski commented Aug 4, 2024

muzarski commented Aug 4, 2024

Lorak-mmk commented Aug 4, 2024

muzarski commented Aug 5, 2024

Lorak-mmk commented Aug 5, 2024 • edited Loading

muzarski commented Aug 5, 2024

Lorak-mmk commented Aug 5, 2024

muzarski commented Aug 5, 2024

muzarski commented Aug 5, 2024

muzarski commented Aug 5, 2024

muzarski commented Aug 7, 2024

muzarski commented Aug 7, 2024

muzarski commented Sep 30, 2024

Lorak-mmk Oct 1, 2024

Choose a reason for hiding this comment

muzarski commented Oct 1, 2024

muzarski commented Aug 1, 2024 •

edited

Loading

Lorak-mmk commented Aug 5, 2024 •

edited

Loading