Please review this commit as I want to make sure I didn’t make any mistakes.
Ralph > On Nov 10, 2017, at 6:46 AM, rgo...@apache.org wrote: > > Repository: logging-log4j2 > Updated Branches: > refs/heads/master 0dca987fc -> aad2f132b > > > LOG4J2-2106 Reduce locking when checking for rollover > > > Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo > Commit: http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/aad2f132 > Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/aad2f132 > Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/aad2f132 > > Branch: refs/heads/master > Commit: aad2f132b27f9e2667c2b43fb58ce59e3914edb3 > Parents: 0dca987 > Author: Ralph Goers <rgo...@apache.org> > Authored: Fri Nov 10 06:46:11 2017 -0700 > Committer: Ralph Goers <rgo...@apache.org> > Committed: Fri Nov 10 06:46:11 2017 -0700 > > ---------------------------------------------------------------------- > .../appender/rolling/RollingFileManager.java | 57 +++++++++++++------- > .../rolling/SizeBasedTriggeringPolicy.java | 2 +- > .../rolling/TimeBasedTriggeringPolicy.java | 2 +- > src/changes/changes.xml | 3 ++ > 4 files changed, 42 insertions(+), 22 deletions(-) > ---------------------------------------------------------------------- > > > http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/aad2f132/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 6ccfe7b..ff7bf6d 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 > @@ -29,6 +29,9 @@ import java.util.concurrent.Semaphore; > import java.util.concurrent.ThreadPoolExecutor; > import java.util.concurrent.TimeUnit; > import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; > +import java.util.concurrent.locks.Lock; > +import java.util.concurrent.locks.ReadWriteLock; > +import java.util.concurrent.locks.ReentrantReadWriteLock; > > import org.apache.logging.log4j.core.Layout; > import org.apache.logging.log4j.core.LifeCycle; > @@ -65,6 +68,9 @@ public class RollingFileManager extends FileManager { > private volatile boolean initialized = false; > private volatile String fileName; > private final FileExtension fileExtension; > + private final ReadWriteLock updateLock = new ReentrantReadWriteLock(); > + private final Lock readLock = updateLock.readLock(); > + private final Lock writeLock = updateLock.writeLock(); > > /* This executor pool will create a new Thread for every work async > action to be performed. Using it allows > us to make sure all the Threads are completed when the Manager is > stopped. */ > @@ -247,9 +253,14 @@ public class RollingFileManager extends FileManager { > * Determines if a rollover should occur. > * @param event The LogEvent. > */ > - public synchronized void checkRollover(final LogEvent event) { > - if (triggeringPolicy.isTriggeringEvent(event)) { > - rollover(); > + public void checkRollover(final LogEvent event) { > + readLock.lock(); > + try { > + if (triggeringPolicy.isTriggeringEvent(event)) { > + rollover(); > + } > + } finally { > + readLock.unlock(); > } > } > > @@ -333,25 +344,31 @@ public class RollingFileManager extends FileManager { > } > > public void setTriggeringPolicy(final TriggeringPolicy triggeringPolicy) { > - triggeringPolicy.initialize(this); > - final TriggeringPolicy policy = this.triggeringPolicy; > - int count = 0; > - boolean policyUpdated = false; > - do { > - ++count; > - } while (!(policyUpdated = > triggeringPolicyUpdater.compareAndSet(this, this.triggeringPolicy, > triggeringPolicy)) > - && count < MAX_TRIES); > - if (policyUpdated) { > - if (triggeringPolicy instanceof LifeCycle) { > - ((LifeCycle) triggeringPolicy).start(); > - } > - if (policy instanceof LifeCycle) { > - ((LifeCycle) policy).stop(); > + writeLock.lock(); > + try { > + triggeringPolicy.initialize(this); > + final TriggeringPolicy policy = this.triggeringPolicy; > + int count = 0; > + boolean policyUpdated = false; > + do { > + ++count; > } > - } else { > - if (triggeringPolicy instanceof LifeCycle) { > - ((LifeCycle) triggeringPolicy).stop(); > + while (!(policyUpdated = > triggeringPolicyUpdater.compareAndSet(this, this.triggeringPolicy, > triggeringPolicy)) > + && count < MAX_TRIES); > + if (policyUpdated) { > + if (triggeringPolicy instanceof LifeCycle) { > + ((LifeCycle) triggeringPolicy).start(); > + } > + if (policy instanceof LifeCycle) { > + ((LifeCycle) policy).stop(); > + } > + } else { > + if (triggeringPolicy instanceof LifeCycle) { > + ((LifeCycle) triggeringPolicy).stop(); > + } > } > + } finally { > + writeLock.unlock(); > } > } > > > http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/aad2f132/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/SizeBasedTriggeringPolicy.java > ---------------------------------------------------------------------- > diff --git > a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/SizeBasedTriggeringPolicy.java > > b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/SizeBasedTriggeringPolicy.java > index f77d571..81069fa 100644 > --- > a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/SizeBasedTriggeringPolicy.java > +++ > b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/SizeBasedTriggeringPolicy.java > @@ -73,7 +73,7 @@ public class SizeBasedTriggeringPolicy extends > AbstractTriggeringPolicy { > * @return true if a rollover should take place, false otherwise. > */ > @Override > - public boolean isTriggeringEvent(final LogEvent event) { > + public synchronized boolean isTriggeringEvent(final LogEvent event) { > final boolean triggered = manager.getFileSize() > maxFileSize; > if (triggered) { > manager.getPatternProcessor().updateTime(); > > http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/aad2f132/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/TimeBasedTriggeringPolicy.java > ---------------------------------------------------------------------- > diff --git > a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/TimeBasedTriggeringPolicy.java > > b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/TimeBasedTriggeringPolicy.java > index 7f6ac79..953e186 100644 > --- > a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/TimeBasedTriggeringPolicy.java > +++ > b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/TimeBasedTriggeringPolicy.java > @@ -122,7 +122,7 @@ public final class TimeBasedTriggeringPolicy extends > AbstractTriggeringPolicy { > * @return true if a rollover should occur. > */ > @Override > - public boolean isTriggeringEvent(final LogEvent event) { > + public synchronized boolean isTriggeringEvent(final LogEvent event) { > if (manager.getFileSize() == 0) { > return false; > } > > http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/aad2f132/src/changes/changes.xml > ---------------------------------------------------------------------- > diff --git a/src/changes/changes.xml b/src/changes/changes.xml > index d54484a..2bcefb2 100644 > --- a/src/changes/changes.xml > +++ b/src/changes/changes.xml > @@ -31,6 +31,9 @@ > - "remove" - Removed > --> > <release version="2.10.0" date="2017-MM-DD" description="GA Release > 2.10.0"> > + <action issue="LOG4J2-2106" dev="rogers" type="fix"> > + Reduce locking when checking for rollover. > + </action> > <action issue="LOG4J2-2103" dev="mikes" type="add"> > XML encoding for PatternLayout. > </action> > >
