Repository: logging-log4j2 Updated Branches: refs/heads/master 821196f87 -> d001230d2
LOG4J-1653 - CronTriggeringPolicy would use the wrong date/time when rolling over and create multiple triggering policies on reconfiguration Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo Commit: http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/d001230d Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/d001230d Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/d001230d Branch: refs/heads/master Commit: d001230d283c54fdfbd2ac8c9e0e8788de0cf315 Parents: 821196f Author: Ralph Goers <[email protected]> Authored: Sun Jan 1 17:16:09 2017 -0700 Committer: Ralph Goers <[email protected]> Committed: Sun Jan 1 17:16:09 2017 -0700 ---------------------------------------------------------------------- .../appender/rolling/CronTriggeringPolicy.java | 37 ++++++++++---------- .../rolling/DefaultRolloverStrategy.java | 6 +++- .../appender/rolling/RollingFileManager.java | 16 ++++++++- .../core/config/ConfigurationScheduler.java | 15 ++++++-- .../logging/log4j/core/util/CronExpression.java | 36 ++++++++++++++----- .../src/test/resources/log4j-rolling-cron.xml | 2 +- 6 files changed, 81 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/d001230d/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java index 28cbd90..b503ac9 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java @@ -23,6 +23,7 @@ import java.util.Objects; import java.util.concurrent.TimeUnit; import org.apache.logging.log4j.core.Core; +import org.apache.logging.log4j.core.LifeCycle; import org.apache.logging.log4j.core.LogEvent; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.ConfigurationScheduler; @@ -46,7 +47,7 @@ public final class CronTriggeringPolicy extends AbstractTriggeringPolicy { private final CronExpression cronExpression; private final Configuration configuration; private final boolean checkOnStartup; - private volatile Date nextRollDate; + private volatile Date lastRollDate; private CronScheduledFuture<?> future; private CronTriggeringPolicy(final CronExpression schedule, final boolean checkOnStartup, @@ -65,22 +66,24 @@ public final class CronTriggeringPolicy extends AbstractTriggeringPolicy { @Override public void initialize(final RollingFileManager aManager) { this.manager = aManager; - final Date nextDate = new Date(this.manager.getFileTime()); - nextRollDate = cronExpression.getNextValidTimeAfter(nextDate); - if (checkOnStartup) { - if (nextRollDate.getTime() < System.currentTimeMillis()) { - rollover(); - } + final Date now = new Date(); + final Date lastRollForFile = cronExpression.getPrevFireTime(new Date(this.manager.getFileTime())); + final Date lastRegularRoll = cronExpression.getPrevFireTime(new Date()); + if (checkOnStartup && lastRollForFile.before(lastRegularRoll)) { + lastRollDate = lastRollForFile; + rollover(); } + final ConfigurationScheduler scheduler = configuration.getScheduler(); - if (!scheduler.isStarted()) { - scheduler.incrementScheduledItems(); - scheduler.start(); - } else if (scheduler.isStarted()) { + if (!scheduler.isExecutorServiceSet()) { // make sure we have a thread pool scheduler.incrementScheduledItems(); } - future = scheduler.scheduleWithCron(cronExpression, new CronTrigger()); + if (!scheduler.isStarted()) { + scheduler.start(); + } + lastRollDate = lastRegularRoll; + future = scheduler.scheduleWithCron(cronExpression, now, new CronTrigger()); } /** @@ -139,13 +142,11 @@ public final class CronTriggeringPolicy extends AbstractTriggeringPolicy { } private void rollover() { - manager.getPatternProcessor().setPrevFileTime(nextRollDate.getTime()); + manager.getPatternProcessor().setPrevFileTime(lastRollDate.getTime()); manager.rollover(); - final Date fireDate = future != null ? future.getFireTime() : new Date(); - final Calendar cal = Calendar.getInstance(); - cal.setTime(fireDate); - cal.add(Calendar.SECOND, -1); - nextRollDate = cal.getTime(); + if (future != null) { + lastRollDate = future.getFireTime(); + } } @Override http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/d001230d/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java index ea32068..df79d1e 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java @@ -538,9 +538,13 @@ public class DefaultRolloverStrategy extends AbstractRolloverStrategy { break; } } + if (currentFileName.equals(renameTo)) { + LOGGER.warn("Attempt to rename file {} to itself will be ignored", currentFileName); + return new RolloverDescriptionImpl(currentFileName, false, null, null); + } final FileRenameAction renameAction = new FileRenameAction(new File(currentFileName), new File(renameTo), - manager.isRenameEmptyFiles()); + manager.isRenameEmptyFiles()); final Action asyncAction = merge(compressAction, customActions, stopCustomActionsOnError); return new RolloverDescriptionImpl(currentFileName, false, renameAction, asyncAction); http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/d001230d/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java index c157b18..9dc7a16 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java @@ -54,6 +54,7 @@ public class RollingFileManager extends FileManager { private volatile TriggeringPolicy triggeringPolicy; private volatile RolloverStrategy rolloverStrategy; private volatile boolean renameEmptyFiles = false; + private volatile boolean initialized = false; private static final AtomicReferenceFieldUpdater<RollingFileManager, TriggeringPolicy> triggeringPolicyUpdater = AtomicReferenceFieldUpdater.newUpdater(RollingFileManager.class, TriggeringPolicy.class, "triggeringPolicy"); @@ -101,7 +102,13 @@ public class RollingFileManager extends FileManager { } public void initialize() { - triggeringPolicy.initialize(this); + if (!initialized) { + initialized = true; + triggeringPolicy.initialize(this); + if (triggeringPolicy instanceof LifeCycle) { + ((LifeCycle) triggeringPolicy).start(); + } + } } /** @@ -214,7 +221,14 @@ public class RollingFileManager extends FileManager { public void setTriggeringPolicy(final TriggeringPolicy triggeringPolicy) { triggeringPolicy.initialize(this); + TriggeringPolicy policy = this.triggeringPolicy; triggeringPolicyUpdater.compareAndSet(this, this.triggeringPolicy, triggeringPolicy); + if (triggeringPolicy instanceof LifeCycle) { + ((LifeCycle) triggeringPolicy).start(); + } + if (policy instanceof LifeCycle) { + ((LifeCycle) policy).stop(); + } } public void setRolloverStrategy(final RolloverStrategy rolloverStrategy) { http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/d001230d/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationScheduler.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationScheduler.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationScheduler.java index d57e5cb..f54fc49 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationScheduler.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationScheduler.java @@ -54,7 +54,7 @@ public class ConfigurationScheduler extends AbstractLifeCycle { return true; } - private boolean isExecutorServiceSet() { + public boolean isExecutorServiceSet() { return executorService != null; } @@ -111,7 +111,18 @@ public class ConfigurationScheduler extends AbstractLifeCycle { * @return a ScheduledFuture representing the next time the command will run. */ public CronScheduledFuture<?> scheduleWithCron(final CronExpression cronExpression, final Runnable command) { - final Date fireDate = cronExpression.getNextValidTimeAfter(new Date()); + return scheduleWithCron(cronExpression, new Date(), command); + } + + /** + * Creates and executes an action that first based on a cron expression. + * @param cronExpression the cron expression describing the schedule. + * @param startDate The time to use as the time to begin the cron expression. Defaults to the current date and time. + * @param command The Runnable to run, + * @return a ScheduledFuture representing the next time the command will run. + */ + public CronScheduledFuture<?> scheduleWithCron(final CronExpression cronExpression, final Date startDate, final Runnable command) { + final Date fireDate = cronExpression.getNextValidTimeAfter(startDate == null ? new Date() : startDate); final CronRunnable runnable = new CronRunnable(command, cronExpression); final ScheduledFuture<?> future = schedule(runnable, nextFireInterval(fireDate), TimeUnit.MILLISECONDS); final CronScheduledFuture<?> cronScheduledFuture = new CronScheduledFuture<>(future, fireDate); http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/d001230d/log4j-core/src/main/java/org/apache/logging/log4j/core/util/CronExpression.java ---------------------------------------------------------------------- diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/CronExpression.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/CronExpression.java index 542f8d9..8adfad8 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/CronExpression.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/CronExpression.java @@ -1162,7 +1162,6 @@ public final class CronExpression { boolean gotOne = false; // loop until we've computed the next time, or we've past the endTime while (!gotOne) { - //if (endTime != null && cl.getTime().after(endTime)) return null; if (cl.get(Calendar.YEAR) > 2999) { // prevent endless loop... return null; @@ -1565,13 +1564,34 @@ public final class CronExpression { } } - /** - * NOT YET IMPLEMENTED: Returns the time before the given time - * that the <code>CronExpression</code> matches. - */ - public Date getTimeBefore(final Date endTime) { - // FUTURE_TODO: implement QUARTZ-423 - return null; + protected Date getTimeBefore(Date targetDate) { + Calendar cl = Calendar.getInstance(getTimeZone()); + + // to match this + Date now = new Date(); + Date nextFireTime = getTimeAfter(now); + Date secondTime = getTimeAfter(nextFireTime); + long interval = secondTime.getTime() - nextFireTime.getTime(); + Date prevCheckDate = new Date(now.getTime() - interval); + Date prevFireTime = getTimeAfter(prevCheckDate); + return prevFireTime; + } + + public Date getPrevFireTime(Date targetDate) { + return getTimeBefore(targetDate); + } + + private int findIncrement(String[] expression) { + // * * * * * * * + // [0]SEC [1]MIN [2]HOUR [3]DAYOFMONTH [4]MONTH [5]DAYOFWEEK [6]YEAR + for (int i = 0; i < expression.length; i++) { + if (expression[i].equals("*")) { + return i; + } + } + + // we default to year TODO: optimize + return YEAR; } /** http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/d001230d/log4j-core/src/test/resources/log4j-rolling-cron.xml ---------------------------------------------------------------------- diff --git a/log4j-core/src/test/resources/log4j-rolling-cron.xml b/log4j-core/src/test/resources/log4j-rolling-cron.xml index be6fa70..4420853 100644 --- a/log4j-core/src/test/resources/log4j-rolling-cron.xml +++ b/log4j-core/src/test/resources/log4j-rolling-cron.xml @@ -16,7 +16,7 @@ limitations under the License. --> -<Configuration status="ERROR" name="RollingCronTest" monitorInterval="1"> +<Configuration status="WARN" name="RollingCronTest" monitorInterval="1"> <Properties> <Property name="filename">target/rolling-cron/rollingtest.log</Property> </Properties>
