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
Fix #8067 Use nanotime for DosFilter rate tracker #8082
Conversation
Use nano time to avoid false positives when wall clock changes.
@@ -1235,7 +1235,7 @@ public OverLimit isRateExceeded(long now) | |||
} | |||
|
|||
long rate = (now - last); | |||
if (rate < 1000L) | |||
if (TimeUnit.NANOSECONDS.toSeconds(rate) < 1L) | |||
{ | |||
return new Overage(Duration.ofMillis(rate), _maxRequestsPerSecond); |
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.
Why is this still using Duration.ofMillis
?
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.
oops
@@ -326,7 +326,7 @@ protected void doFilter(HttpServletRequest request, HttpServletResponse response | |||
tracker = getRateTracker(request); | |||
|
|||
// Calculate the rate and check if it is over the allowed limit | |||
final OverLimit overLimit = tracker.isRateExceeded(System.currentTimeMillis()); | |||
final OverLimit overLimit = tracker.isRateExceeded(System.nanoTime()); |
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.
Can you update the javadoc of isRateExceeded
. It specifies that the value is in ms.
now the time now (in milliseconds)
updates from review
@@ -1216,7 +1216,7 @@ public RateTracker(ServletContext context, String filterName, String id, RateTyp | |||
} | |||
|
|||
/** | |||
* @param now the time now (in milliseconds) | |||
* @param now the time now (in nanoseconds) |
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.
Could you please mention elapsed time or something in the javadoc as a hint that nanoTime()
should be used instead of currentTimeMillis()
?
@lachlan-roberts can you rereview to remove your objection. |
I think the CI failure is some kind of CI problem. Distribution tests work locally |
Use nano time to avoid false positives when wall clock changes.