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

Avoid usage of the non monotonic clock System.currentTimeMillis() in favor of System.nanoTime() #6392

Merged
merged 6 commits into from
Mar 14, 2024

Conversation

Nateckert
Copy link
Contributor

In Java, System.currentTimeMillis() is a non-monotonic clock, which means that

long start = System.currentTimeMillis();
...
long end = System.currentTimeMillis();

System.println.out(end - start);

might print a number smaller than zero !

This is due to the fact that the internal clock of computer if not accurate and it is put back on the "right" time thanks to the UDP protocol.

Additional read: https://superuser.com/questions/759730/how-much-clock-drift-is-considered-normal-for-a-non-networked-windows-7-pc

Instead, System.nanoTime() should be prefer for timing purpose, as it will guarantee that two successive calls to it are greater than zero

@Nateckert Nateckert requested a review from a team as a code owner January 4, 2023 17:07
@Nateckert
Copy link
Contributor Author

@Nateckert Nateckert changed the title Avoid usage of the non monotonic clock System.currentTimeMillis() in favor for System.nanoTime() Avoid usage of the non monotonic clock System.currentTimeMillis() in favor of System.nanoTime() Jan 4, 2023
@kiview
Copy link
Member

kiview commented Jan 5, 2023

Thanks for providing a PR @Nateckert. Is this an issue you have observed in your system while using Testcontainers?

Could you please run ./gradlew :database-commons:spotlessApply to fix the failing CI job?

@Nateckert
Copy link
Contributor Author

Thanks for providing a PR @Nateckert. Is this an issue you have observed in your system while using Testcontainers?

Could you please run ./gradlew :database-commons:spotlessApply to fix the failing CI job?

I ran the formatter.

I actually don't know if we encounter that issue, but we are currently trying to sanitize our CI and this was a quick fix to a unlikely problem that can be almost impossible to debug (looks random, there is no associated logs, ...), so that's why I wanted to contribute


waitUntil(predicate, expiry, times);
}

private void waitUntil(Predicate<OutputFrame> predicate, long expiry, int times) throws TimeoutException {
int numberOfMatches = 0;
while (System.currentTimeMillis() < expiry) {
while (System.nanoTime() < expiry) {
Copy link

@jtnord jtnord Jan 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nanoTime is subject to overflow/wrapping and the addition of an offset may have wrapped.

Whilst this could have happened with mills you wouldn't care for a few more years. nanoTime is relative to a random point in time so it could be a very large number or a very small number, positive or megative

As per javadoc

To compare two nanoTime values

long t0 = System.nanoTime();
...
long t1 = System.nanoTime();
one should use t1 - t0 < 0, not t1 < t0, because of the possibility of numerical overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@Nateckert
Copy link
Contributor Author

Nateckert commented Feb 3, 2023

The comment https://github.com/testcontainers/testcontainers-java/pull/6392/files#diff-1cb71d0ebd85bd607c3da0c4e693f353e5d0b9c947a2396ffc32042f3ccbb63eL45 is now incorrect, but a good estimate is hard to guess

With the changes after @jtnord's feedback, I managed to update the estimate

kiview
kiview previously approved these changes Mar 6, 2023
@kiview kiview requested a review from eddumelendez March 6, 2023 14:16
@eddumelendez eddumelendez merged commit 6e5fbe6 into testcontainers:main Mar 14, 2024
99 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @Nateckert !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants