Time used fetching should never be negative #878
Conversation
| @@ -149,7 +149,7 @@ private List<Message> _fetch(int batchSize, long maxWaitMillis) { | |||
| batchLeft--; | |||
| } | |||
| // try again while we have time | |||
| timeLeft = maxWaitMillis - (System.currentTimeMillis() - start); | |||
| timeLeft = maxWaitMillis - Math.max(0, System.currentTimeMillis() - start); | |||
There was a problem hiding this comment.
The right side will always be zero, which also doesn't work, timeLeft will never decrease below maxWaitMillis.
There was a problem hiding this comment.
This is my proposal.
timeLeft = maxWaitMillis - Math.abs(System.currentTimeMillis() - start);
There was a problem hiding this comment.
The Math.abs solution will make timeLeft < 0 if the clock changes backwards, stopping the loop. Messages that are missed will get put in a buffer and dealt with the next time fetch is called again, see lines 123-127. The new underlying pull will be adjusted.
Just a note regarding what we are calling simplification. I'm making a new "fetch" (really an iterate, it does not block and also making a pull based consumer that is endless, which means it looks like a push subscription but uses pull under the covers. These may or may not be of interest to you, either way I should have a beta by end of week.
There was a problem hiding this comment.
Yeah, definitely not ideal. Alternatively we could reset and let the user handle it? Since having the current time be less than the original start time is invalid anyway.
There was a problem hiding this comment.
The Math.abs version will do the trick. If the current time is less than start, the loop will stop. If the time changes forward, the right side is still valid and makes timeLeft less than 0, also stopping.
If you would like to make the changes, please go ahead, otherwise I will make a PR. If you can, please verify your commit. https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
There was a problem hiding this comment.
Should we not move away from using System.currentTimeMillis() and use something like System.nanoTime()?
If device system time changes into the past while fetching from nats, we should not allow the difference in time to be negative. This would cause the call to nextMessageInternal to hang for a long time.