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
Allow to schedule tasks up to Long.MAX_VALUE #7972
Conversation
a1ca3ac
to
a0cc1c8
Compare
@carl-mastrangelo @ejona86 I think this is the correct fix. |
@@ -237,7 +236,7 @@ private int epollWait(boolean oldWakeup) throws IOException { | |||
long totalDelay = delayNanos(System.nanoTime()); | |||
int delaySeconds = (int) min(totalDelay / 1000000000L, Integer.MAX_VALUE); | |||
return Native.epollWait(epollFd, events, timerFd, delaySeconds, | |||
(int) min(totalDelay - delaySeconds * 1000000000L, Integer.MAX_VALUE)); | |||
(int) min(MAX_SCHEDULED_TIMERFD_NS, min(totalDelay - delaySeconds * 1000000000L, Integer.MAX_VALUE))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified to drop Integer.MAX_VALUE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry not sure I understand... what exactly you propose ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Native.epollWait(epollFd, events, timerFd, delaySeconds,
(int) min(MAX_SCHEDULED_TIMERFD_NS, totalDelay - delaySeconds * 1000000000L));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah... let me do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carl-mastrangelo PTAL again
Motivation: We should allow to schedule tasks with a delay up to Long.MAX_VALUE as we did pre 4.1.25.Final. Modifications: Just ensure we not overflow and put the correct max limits in place when schedule a timer. At worse we will get a wakeup to early and then schedule a new timeout. Result: Fixes #7970.
a0cc1c8
to
e05df83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@carl-mastrangelo can you validate this one with GRPC as well ? |
@normanmaurer I did try to test this out, sadly its pretty time consuming to bring netty into Google if it isn't a released artifact. I think code review is good enough here. |
@carl-mastrangelo ok... 😢 |
Motivation:
We should allow to schedule tasks with a delay up to Long.MAX_VALUE as we did pre 4.1.25.Final.
Modifications:
Just ensure we not overflow and put the correct max limits in place when schedule a timer. At worse we will get a wakeup to early and then schedule a new timeout.
Result:
Fixes #7970.