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

Potential issues by using non-monotonic clocks #206

Closed
ghost opened this issue Jan 11, 2023 · 1 comment
Closed

Potential issues by using non-monotonic clocks #206

ghost opened this issue Jan 11, 2023 · 1 comment

Comments

@ghost
Copy link

ghost commented Jan 11, 2023

There are a few invocations of non-monotonic clocks that may require closer examination. These include:

            Message.marshallintBig(System.currentTimeMillis(), buf, 0, 8);
// ...
            long start = System.currentTimeMillis();
// ...
            while (lCookie == null && (System.currentTimeMillis() - start) < LOCK_TIMEOUT) {

This one would depend on how the marshalled ID is used:

                    long id = System.currentTimeMillis();
                    byte[] buf = new byte[8];
                    Message.marshallintBig(id, buf, 0, 8);
  • EmbeddedDBusDaemon::startInBackgroundAndWait, RunDaemon::startDaemon, and RunTwoPartDaemon::startDaemon have effectively duplicate code blocks for busy-waits. I think startInBackgroundAndWait can be removed. If not, consider refactoring the duplication to make finding and reviewing the code for potential timing-related issues easier.
  • The unit tests AbstractBaseTest and AbstractDBusBaseTest also duplicate the same code. These could be refactored to simplify the code base and make finding potential timing-related issues easier.

I couldn't find anything else of note with respect to non-monotonic time.

@hypfvieh
Copy link
Owner

I removed the usage of System.currentMillis().
In case where some sort of "wait for lock" was used, I now use TimeMeasure. It was originally intended for debugging only (measuring runtimes). I updated the code of TimeMeasure to use System.nanoTime() and added the required function for the measuring the locking stuff.

Regarding the duplicated code: No EmbeddedDBusDaemon::startInBackgroundAndWait cannot be removed. It is public API.
I moved the wait stuff to a more generic method in Util. Now the Util.waitFor method is called when waiting for something is required.

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

No branches or pull requests

1 participant