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

refactor: Make SystemTimeTools::shift() async #5450

Closed
wants to merge 1 commit into from

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Apr 9, 2024

It's a more stable interface, if shift() suddenly will need to wait for smth like finalisation of
parallel running critical sections assuming there are no time jumps, we don't need to fix all its
usages.

deltachat-time/src/lib.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo marked this pull request as ready for review April 9, 2024 08:08
@iequidoo iequidoo requested a review from link2xt April 9, 2024 08:09
@link2xt
Copy link
Collaborator

link2xt commented Apr 9, 2024

Using tokio Mutex instead of sync Mutex is probably unnecessary?
https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use

@iequidoo iequidoo force-pushed the iequidoo/test_was_seen_recently branch from 84392ea to 8b36603 Compare April 10, 2024 01:36
@iequidoo
Copy link
Collaborator Author

Using tokio Mutex instead of sync Mutex is probably unnecessary? https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use

Probably you're talking about RwLock-s. Just forgot to change it back after my experiments when it appeared that the problem can't be solved with the only Tokio's RwLock. Done now, added a comment on that also.

@iequidoo
Copy link
Collaborator Author

And also replaced Tokio's Mutex with RwLock so that parallel critical sections using SystemTimeTools::lock() are possible.

@iequidoo iequidoo force-pushed the iequidoo/test_was_seen_recently branch from 8b36603 to c66cd93 Compare April 10, 2024 02:53
@iequidoo iequidoo requested a review from Hocuri April 10, 2024 02:54
Cargo.toml Outdated Show resolved Hide resolved
@Hocuri
Copy link
Collaborator

Hocuri commented Apr 10, 2024

The fact that cargo test runs multiple tests in the same process has caused us so many problems (also #2901, and I think that there was some other problem that I can't remember right now) that I'm wondering whether it would be better to say that everyone needs to run tests via cargo-nextest. Which would have the additional advantage of running the tests slightly faster (for faster CI response time).

OTOH, there would probably a lot of things to be taken care of, like installing nextest in the CI, running doc tests with cargo test --doc, making sure that tests in all workspaces are run, and making nextest work with https://github.com/dignifiedquire/cross which we are using apparently (TIL by reading scripts/run-rust-test.sh).

What do you think?

deltachat-time/src/lib.rs Outdated Show resolved Hide resolved
@link2xt
Copy link
Collaborator

link2xt commented Apr 10, 2024

Alternative solution could be to store separate shift for each test by storing a map from tokio runtime ID (https://docs.rs/tokio/1.37.0/tokio/runtime/struct.Handle.html#method.id) to time shift. Then tests have independent time shift even if they run concurrently and there is no need to take care of "locking time" in the tests. Using Id unfortunately requires enabling tokio_unstable feature, but we have a Cargo.lock file, this will not accidentally break.

Switching CI to cargo nextest probably makes sense, iroh is using it: n0-computer/iroh#1867
But I am not sure about breaking compatibility with cargo test now if this issue can be fixed.

@link2xt
Copy link
Collaborator

link2xt commented Apr 10, 2024

and making nextest work with https://github.com/dignifiedquire/cross which we are using apparently (TIL by reading scripts/run-rust-test.sh).

We don't run cross anywhere in CI and don't use it for cross-compilation, it could be dropped everywhere.

@link2xt
Copy link
Collaborator

link2xt commented Apr 10, 2024

For switching to nextest I opened #5457

It's a more stable interface, if `shift()` suddenly will need to wait for smth like finalisation of
parallel running critical sections assuming there are no time jumps, we don't need to fix all its
usages.
@iequidoo iequidoo force-pushed the iequidoo/test_was_seen_recently branch from c66cd93 to 796d34f Compare April 11, 2024 01:46
@iequidoo iequidoo changed the title test: Protect from time jumps in the tests (#5440) refactor: Make SystemTimeTools::shift() async Apr 11, 2024
@iequidoo
Copy link
Collaborator Author

Removed SystemTimeTools::lock() as it's useless currently, but left shift() async just in case

@iequidoo
Copy link
Collaborator Author

Let's close this. Can be reopened if will be really needed

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.

3 participants