fix: Use SystemTime instead of Instant everywhere

[iequidoo]

See commit messages.

[iequidoo]

I'm going to mock the system clock in some way for the tests so that we can write tests checking timestamps of messages and this is some preparational work. The reason is that we already have some fixes like #5094 that are not covered by any tests because such tests would need large sleep()s currently

[r10s]

did not look closely to the pr, just by the title: InstantTime had issues in the past, therefore we switched to SystemTime: #1706

only tests seems to be a weak reason to change these things, that could maybe achieved also differently.

do we really have bugs wrt time? if so, i'd fix them more on-point; it seems to be so easy to break things in this area :) otherwise, never change a running system :)

[iequidoo]

did not look closely to the pr, just by the title: InstantTime had issues in the past, therefore we switched to SystemTime: #1706

only tests seems to be a weak reason to change these things, that could maybe achieved also differently.

do we really have bugs wrt time? if so, i'd fix them more on-point; it seems to be so easy to break things in this area :) otherwise, never change a running system :)

You're right, the issue with Instant on Android is still unresolved: rust-lang/rust#88714. But even before this PR Instant is used in several places, they also need to switch to SystemTime. But just using SystemTime in places where a monotonic clock is really needed doesn't look good. At least this confuses the reader. So, i still suggest to switch to Instant in these places, but make it an alias of SystemTime for now. Then, when the issue on Android will be solved, we can switch to using the true Instant easily.

As for the second commit, it doesn't change much, i only cache a system time value if it's used several times in a function and isn't supposed to change. In some places it's already done currently, i just want the same approach to be used everywhere.

[iequidoo]

Switched to using SystemTime instead of Instant in the remaining places (except tests) by making it an alias of SystemTime and added a comment why this approach is used.

At least, now it's clear where a real monotonic clock should be used, but can't be used currently.

[Hocuri]

I'm a bit unsure which problem this PR is trying to fix. Sure, using SystemTime in places where you should use Instant is confusing, but I don't see how calling it Instant but actually making it a SystemTime makes this better. If we do want to centralize it (e.g. to be able to mock system time in tests - note that Tokio has support for mocking system time, too) Edit: Just saw it, it's preparations for mocking system time

I'm not sure we should call it Instant if it's not the same as std's Instant. If we want to mock the system time, we'll of course still need some central place for time stuff, it just seems weird to call it Instant if it's not the Instant people will assume it is - then again, tokio also calls theirs Instant. I any case, let's not assume a future where std's Instant is fixed, since there wasn't any movement on rust-lang/rust#71860 for over 3 years.

For the case that you haven't seen it already: tokio has some support for mocking the system time, maybe it's helpful.

For the second commit, are there any reports that there are problems with decreasing time?

[link2xt]

Then, when the issue on Android will be solved

It is not going to be solved according to the discussion and it is impossible to solve on Android 4.1 phones with kernels that don't support CLOCK_BOOTTIME. So we should work with the constraints of SystemTime being a system time that can jump back and forth even by hours sometimes and Instant being monotonic but freezing during system sleep.

[iequidoo]

I'm not sure we should call it Instant if it's not the same as std's Instant.

Ok, agree with this.

For the second commit, are there any reports that there are problems with decreasing time?

There are probably no such reports, but some functions cache the system time in a var named e.g. now, but others don't do that so the system time can jump back and forth for them. In most cases it's not a big problem, but using the same approach everywhere looks better and in some places like export_backup() i'd prefer a formatted time in the backup name to correspond to the backup_time config value inside the backup. Currently they can differ.

So we should work with the constraints of SystemTime being a system time that can jump back and forth even by hours sometimes and Instant being monotonic but freezing during system sleep.

So, we're not going to get rid of using std::time::Instant completely, but only in those places where such freezes are unacceptable? Then all usages of Instant should be reviewed, probably some of them switched to SystemTime and explanation comments added about why Instant isn't used in these palces. What i'm trying to solve is that after @r10s's #1706 some new unwanted usages of Instant were added, probably by people not aware of the Android Instant issue, at least me.

[iequidoo]

I checked the @r10s's commit again and as i see the initial idea was to get rid of Instant at all because it can freeze on some platforms like Android. All usages of Instant were removed at that time. But then they started to appear again :) I'd also vote for not using Instant at all, otherwise every time we need to remember about these freezes and think how critical it is in a particular place. But it should be made in some central place in the code with the appropriate comment added, otherwise the problem will reappear again in the future. So, what is wrong with my commit is that i called it Instant while it's actually SystemTime -- maybe it should be tools::Time then -- and the comments / commit message should be improved.

[iequidoo]

For the case that you haven't seen it already: tokio has some support for mocking the system time, maybe it's helpful.

For some reason i only see tokio::time::pause() and advance(), but they're about Instant, and don't see any SystemTime there at all.

Btw, there's another problem with mocking the system time for tests -- they run in parallel and a system time adjustment in one test can break another one. So, just mocking now() and elapsed() is easy, but not the right way. I didn't come up yet with any solution other than forwarding Context to all functions watching at the system clock, but that would be a huge change.

UPD: I think that tests running in parallel are not a problem however. Probably currently we don't have tests that may fail because of the advancing system time. And as for new tests on message timestamps, they just must assume that the system time can jump forth, so there must be more careful timestamp checks but otherwise this looks like a working solution.

[link2xt]

tokio has some support for mocking the system time

Maybe it is a reference to turmoil but this is probably an overkill for just mocking time.

[iequidoo]

I've already done with mocking SystemTime::now() and elapsed(), the only thing one should remember is not to call the original elapsed(), but its mock. But it's only for tests, so, the worst thing that can happen is that some test on msg timestamps would miss a bug, i think it's acceptable. Let's see how it works.

Btw, with this elapsed() they really complicated mocking, if they made it a non-member function, mocking would be much easier/robust.