-
Notifications
You must be signed in to change notification settings - Fork 109
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
test_coalescing might be flaky #828
Comments
Hmm, it looks like error propagation is incorrect in that test. It uses We should change |
`test_coalescing` spawns a lot of tokio tasks, each of which runs a query on the database. If a query fails, the task will panic because of the `unwrap()`. The problem is that all of the tasks are joined using `join_all`, which doesn't check for errors in the joined tasks. For example, this piece of code prints "Ok!" and the program finishes successfuly: ```rust let mut tasks = Vec::new(); for _i in 0..4 { tasks.push(tokio::spawn(async { panic!("Panic!") })); } futures::future::join_all(tasks).await; println!("Ok!"); ``` There are some error messages in the output, but the important thig is that they don't stop the control flow. To fix it, let's propagate the errors upwards and use `try_join_all` to join all the tasks. `try_join_all` will check whether the task was successful and fail the test if it isn't. Refs: scylladb#828 Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
I opened #834 to fix the problem with The test failed because one of the queries has failed, but IMO the test looks okay, the failure was most likely caused by external factors. I think we can close this issue and reopen it if it fails again in the future. |
The test is not OK because it does not propagate errors properly. However, I agree that it would still be flaky even if it did. I'll close the issue as suggested. |
`test_coalescing` spawns a lot of tokio tasks, each of which runs a query on the database. If a query fails, the task will panic because of the `unwrap()`. The problem is that all of the tasks are joined using `join_all`, which doesn't check for errors in the joined tasks. For example, this piece of code prints "Ok!" and the program finishes successfuly: ```rust let mut tasks = Vec::new(); for _i in 0..4 { tasks.push(tokio::spawn(async { panic!("Panic!") })); } futures::future::join_all(tasks).await; println!("Ok!"); ``` There are some error messages in the output, but the important thig is that they don't stop the control flow. To fix it, let's use `try_join_all` instead of `join_all`. `try_join_all` will check whether the task panicked or not, and return an error if there was a panic. I manually tested that it works by inserting a panic!() after `conn.query()`. Let's also make sure that the result of join_all doesn't contain any `Result<>`, as it could hide the errors. The result of `join_all` should be just a `Vec<>` of unit values. Refs: scylladb#828 Signed-off-by: Jan Ciolek <jan.ciolek@scylladb.com>
test_coalescing
failed in run https://github.com/scylladb/scylla-rust-driver/actions/runs/6473573762/job/17576595276.It might be a flaky test.
Here's the failure log:
The text was updated successfully, but these errors were encountered: