[
https://issues.apache.org/jira/browse/ARTEMIS-2926?focusedWorklogId=505306&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-505306
]
ASF GitHub Bot logged work on ARTEMIS-2926:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 27/Oct/20 16:34
Start Date: 27/Oct/20 16:34
Worklog Time Spent: 10m
Work Description: gemmellr commented on a change in pull request #3287:
URL: https://github.com/apache/activemq-artemis/pull/3287#discussion_r512832724
##########
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -188,15 +181,27 @@ public ClassLoader run() {
}
- public void delay() {
- int value = delayed.incrementAndGet();
- if (value > 10) {
- delayed.decrementAndGet();
- } else {
- // We only schedule up to 10 periods upfront.
- // this is to avoid a window where a current one would be running and
a next one is coming.
- // in theory just 2 would be enough. I'm using 10 as a precaution
here.
- scheduledExecutorService.schedule(runForScheduler, Math.min(period,
period * value), timeUnit);
+ /**
+ * If {@code overBooking} is {@code true} a delay request can succeed only
if:
+ * <ul>
+ * <li>there is no other pending delay request
+ * <li>there is no pending execution request
+ * </ul>
+ * <p>
+ * If {@code overBooking} is {@code false} a delay request must always
succeed.<br>
+ * When a delay request succeed it schedule a new execution to happen in
{@link #getPeriod()}.<br>
Review comment:
Comment needs updated
##########
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -265,6 +270,9 @@ public void stop() {
if (future != null) {
future.cancel(false);
future = null;
+ // Replace te existing one: a new periodic task or any new delay
after stop
+ // won't interact with the previously running ones
+ this.bookedForRunning = new AtomicBoolean(false);
Review comment:
The comment doesnt entirely fit if this is inside the null check, does
it? It wont apply for a 'new delay' since the future doesnt exist for such 'on
demand' usage.
##########
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -265,6 +270,9 @@ public void stop() {
if (future != null) {
future.cancel(false);
future = null;
+ // Replace te existing one: a new periodic task or any new delay
after stop
Review comment:
"the"
##########
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -287,35 +295,47 @@ private void restartIfNeeded() {
}
}
- final Runnable runForExecutor = new Runnable() {
- @Override
- public void run() {
- if (onDemand && delayed.get() > 0) {
- delayed.decrementAndGet();
- }
+ private void runForExecutor(AtomicBoolean booked) {
+ // It unblocks:
+ // - a new delay request
+ // - next periodic run request (in case of executor != null)
+ // Although tempting, don't move this one after
ActiveMQScheduledComponent.this.run():
+ // - it can cause "delay" to change semantic ie a racing delay while
finished executing the task, won't succeed
+ // - it won't prevent "slow tasks" to accumulate, because slowness
cannot be measured inside running method;
+ // it just cause skipping runs for perfectly timed executions too
+ // Re the code as it is: if the next periodic run has been able to
request a new round while
+ // the previous one hasn't yet started, it makes sense to skip it,
because its job will be executed by
+ // the old one (yet to start).
Review comment:
Not sure this bit of comment is in the right place, or if so could do
with updating. This code here doesnt skip anything, as the comment reads like.
It instead resets the flag used by other areas to skip.
##########
File path:
artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java
##########
@@ -287,35 +295,47 @@ private void restartIfNeeded() {
}
}
- final Runnable runForExecutor = new Runnable() {
- @Override
- public void run() {
- if (onDemand && delayed.get() > 0) {
- delayed.decrementAndGet();
- }
+ private void runForExecutor(AtomicBoolean booked) {
+ // It unblocks:
+ // - a new delay request
+ // - next periodic run request (in case of executor != null)
+ // Although tempting, don't move this one after
ActiveMQScheduledComponent.this.run():
+ // - it can cause "delay" to change semantic ie a racing delay while
finished executing the task, won't succeed
+ // - it won't prevent "slow tasks" to accumulate, because slowness
cannot be measured inside running method;
+ // it just cause skipping runs for perfectly timed executions too
+ // Re the code as it is: if the next periodic run has been able to
request a new round while
+ // the previous one hasn't yet started, it makes sense to skip it,
because its job will be executed by
+ // the old one (yet to start).
+ boolean alwaysTrue = booked.compareAndSet(true, false);
+ assert alwaysTrue;
+ ActiveMQScheduledComponent.this.run();
+ }
- if (!onDemand && lastTime > 0) {
- if (System.currentTimeMillis() - lastTime < millisecondsPeriod) {
- logger.trace("Execution ignored due to too many simultaneous
executions, probably a previous delayed execution");
- return;
+ private void bookedRunForScheduler(AtomicBoolean booked) {
+ if (executor != null) {
+ assert booked.get();
Review comment:
assert is in both legs, move outside if
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 505306)
Time Spent: 2.5h (was: 2h 20m)
> Scheduled task executions are skipped randomly
> ----------------------------------------------
>
> Key: ARTEMIS-2926
> URL: https://issues.apache.org/jira/browse/ARTEMIS-2926
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Components: Broker
> Affects Versions: 2.13.0
> Reporter: Apache Dev
> Assignee: Francesco Nigro
> Priority: Major
> Time Spent: 2.5h
> Remaining Estimate: 0h
>
> Scheduled tasks extending {{ActiveMQScheduledComponent}} could randomly skip
> an execution, logging:
> {code}
> Execution ignored due to too many simultaneous executions, probably a
> previous delayed execution
> {code}
> The problem is in the "ActiveMQScheduledComponent#runForExecutor" Runnable.
> Times to be compared ({{currentTimeMillis()}} and {{lastTime}}) are taken
> inside the runnable execution itself. So, depending on relative execution
> times, it could happen that the difference is less than the given period
> (e.g. 1 ms), resulting in a skipped execution.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
