Skip to content

Commit

Permalink
Rework locking for Loom compatibility
Browse files Browse the repository at this point in the history 
* Change `synchronized` constructs that cover blocking code to use a shared `ReentrantLock`
* Unify discrete locks among `ExtHandler` subclasses
* Remove `protected` `outputLock` object which was used for synchronization among subclasses of `WriterHandler` and replace with shared `lock`
* Protect `setCharset` with lock to retain behavior of `java.util.logging.Handler`
* Shadow private fields from `j.u.l.Handler` which are protected by `synchronized` in that class
* Replace tree lock with `ReentrantLock`
  • Loading branch information
@dmlloyd
dmlloyd committed May 5, 2023
1 parent 6e0fc5f commit e992878
Show file tree
Hide file tree
Showing 13 changed files with 423 additions and 113 deletions.
70 changes: 64 additions & 6 deletions src/main/java/org/jboss/logmanager/ExtHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.security.Permission;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.ErrorManager;
import java.util.logging.Filter;
import java.util.logging.Formatter;
Expand All @@ -44,6 +45,15 @@ public abstract class ExtHandler extends Handler implements AutoCloseable, Flush
private static final ErrorManager DEFAULT_ERROR_MANAGER = new OnlyOnceErrorManager();
private static final Permission CONTROL_PERMISSION = new LoggingPermission("control", null);

protected final ReentrantLock lock = new ReentrantLock();

// we keep our own copies of these fields so that they are protected with *our* lock:
private volatile Filter filter;
private volatile Formatter formatter;
private volatile Level level = Level.ALL;
private volatile ErrorManager errorManager = new ErrorManager();
// (skip `encoding` because we replace it with `charset` below)

private volatile boolean autoFlush = true;
private volatile boolean enabled = true;
private volatile boolean closeChildren;
Expand Down Expand Up @@ -330,13 +340,34 @@ public void close() throws SecurityException {
@Override
public void setFormatter(final Formatter newFormatter) throws SecurityException {
checkAccess();
super.setFormatter(newFormatter);
Objects.requireNonNull(newFormatter);
lock.lock();
try {
formatter = newFormatter;
} finally {
lock.unlock();
}
}

@Override
public Formatter getFormatter() {
return formatter;
}

@Override
public void setFilter(final Filter newFilter) throws SecurityException {
checkAccess();
super.setFilter(newFilter);
lock.lock();
try {
filter = newFilter;
} finally {
lock.unlock();
}
}

@Override
public Filter getFilter() {
return filter;
}

/**
Expand All @@ -349,7 +380,7 @@ public void setFilter(final Filter newFilter) throws SecurityException {
*/
@Override
public void setEncoding(final String encoding) throws SecurityException, UnsupportedEncodingException {
try {
if (encoding != null) try {
setCharset(Charset.forName(encoding));
} catch (IllegalArgumentException e) {
final UnsupportedEncodingException e2 = new UnsupportedEncodingException("Unable to set encoding to \"" + encoding + "\"");
Expand Down Expand Up @@ -388,7 +419,12 @@ public void setCharset(final Charset charset) throws SecurityException {
*/
protected void setCharsetPrivate(final Charset charset) throws SecurityException {
Objects.requireNonNull(charset, "charset");
this.charset = charset;
lock.lock();
try {
this.charset = charset;
} finally {
lock.unlock();
}
}

/**
Expand All @@ -402,14 +438,36 @@ public Charset getCharset() {

@Override
public void setErrorManager(final ErrorManager em) {
Objects.requireNonNull(em);
checkAccess();
super.setErrorManager(em);
lock.lock();
try {
errorManager = em;
} finally {
lock.unlock();
}
}

@Override
public ErrorManager getErrorManager() {
return errorManager;
}

@Override
public void setLevel(final Level newLevel) throws SecurityException {
Objects.requireNonNull(newLevel);
checkAccess();
super.setLevel(newLevel);
lock.lock();
try {
level = newLevel;
} finally {
lock.unlock();
}
}

@Override
public Level getLevel() {
return level;
}

/**
Expand Down
25 changes: 19 additions & 6 deletions src/main/java/org/jboss/logmanager/LogContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Level;
import java.util.logging.LoggingPermission;

Expand Down Expand Up @@ -136,7 +137,7 @@ private static void addStrong(Map<String, Reference<Level, Void>> map, Level lev
/**
* This lock is taken any time a change is made which affects multiple nodes in the hierarchy.
*/
final Object treeLock = new Object();
final ReentrantLock treeLock = new ReentrantLock();

LogContext(final boolean strong, LogContextInitializer initializer) {
this.strong = strong;
Expand Down Expand Up @@ -511,13 +512,16 @@ public static LogContextSelector getLogContextSelector() {

@Override
public void close() throws Exception {
synchronized (treeLock) {
treeLock.lock();
try {
// First we want to close all loggers
recursivelyClose(rootLogger);
// Next process the close handlers associated with this log context
for (AutoCloseable handler : closeHandlers) {
handler.close();
}
} finally {
treeLock.unlock();
}
}

Expand Down Expand Up @@ -548,8 +552,11 @@ public void addCloseHandler(final AutoCloseable closeHandler) {
if (sm != null) {
sm.checkPermission(CONTROL_PERMISSION);
}
synchronized (treeLock) {
treeLock.lock();
try {
closeHandlers.add(closeHandler);
} finally {
treeLock.unlock();
}
}

Expand All @@ -559,8 +566,11 @@ public void addCloseHandler(final AutoCloseable closeHandler) {
* @return the current close handlers
*/
public Set<AutoCloseable> getCloseHandlers() {
synchronized (treeLock) {
treeLock.lock();
try {
return new LinkedHashSet<>(closeHandlers);
} finally {
treeLock.unlock();
}
}

Expand All @@ -579,9 +589,12 @@ public void setCloseHandlers(final Collection<AutoCloseable> closeHandlers) {
if (sm != null) {
sm.checkPermission(CONTROL_PERMISSION);
}
synchronized (treeLock) {
treeLock.lock();
try {
this.closeHandlers.clear();
this.closeHandlers.addAll(closeHandlers);
} finally {
treeLock.unlock();
}
}

Expand Down Expand Up @@ -609,7 +622,7 @@ LogContextInitializer getInitializer() {
}

private void recursivelyClose(final LoggerNode loggerNode) {
assert Thread.holdsLock(treeLock);
assert treeLock.isHeldByCurrentThread();
for (LoggerNode child : loggerNode.getChildren()) {
recursivelyClose(child);
}
Expand Down
23 changes: 16 additions & 7 deletions src/main/java/org/jboss/logmanager/LoggerNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.atomic.AtomicReferenceFieldUpdater;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.ErrorManager;
import java.util.logging.Filter;
import java.util.logging.Handler;
Expand Down Expand Up @@ -217,7 +218,9 @@ static Handler[] safeCloneHandlers(Handler... initialHandlers) {

@Override
public void close() {
synchronized (context.treeLock) {
final ReentrantLock treeLock = context.treeLock;
treeLock.lock();
try {
// Reset everything to defaults
filter = null;
if ("".equals(fullName)) {
Expand All @@ -235,6 +238,8 @@ public void close() {
attachmentKey2 = null;
attachmentValue2 = null;
children.clear();
} finally {
treeLock.unlock();
}
}

Expand Down Expand Up @@ -419,9 +424,9 @@ public ErrorManager run() {
}

void setLevel(final Level newLevel) {
final LogContext context = this.context;
final Object lock = context.treeLock;
synchronized (lock) {
final ReentrantLock treeLock = context.treeLock;
treeLock.lock();
try {
final int oldEffectiveLevel = effectiveLevel;
final int newEffectiveLevel;
if (newLevel != null) {
Expand All @@ -446,6 +451,8 @@ void setLevel(final Level newLevel) {
}
}
}
} finally {
treeLock.unlock();
}
}

Expand Down Expand Up @@ -554,10 +561,12 @@ boolean isLoggable(final ExtLogRecord record) {
final Filter filter = this.filter;
return filter == null || filter.isLoggable(record);
}
final LogContext context = this.context;
final Object lock = context.treeLock;
synchronized (lock) {
final ReentrantLock treeLock = context.treeLock;
treeLock.lock();
try {
return isLoggable(this, record);
} finally {
treeLock.unlock();
}
}

Expand Down
39 changes: 25 additions & 14 deletions src/main/java/org/jboss/logmanager/handlers/DelayedHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ protected void doPublish(final ExtLogRecord record) {
publishToNestedHandlers(record);
super.doPublish(record);
} else {
synchronized (this) {
lock.lock();
try {
// Check one more time to see if we've been activated before queuing the messages
if (activated) {
publishToNestedHandlers(record);
Expand All @@ -154,6 +155,8 @@ protected void doPublish(final ExtLogRecord record) {
}
enqueueOrdered(q, record);
}
} finally {
lock.unlock();
}
}
}
Expand All @@ -165,7 +168,7 @@ protected void doPublish(final ExtLogRecord record) {
* @param record the record
*/
private void enqueueOrdered(Deque<ExtLogRecord> q, ExtLogRecord record) {
assert Thread.holdsLock(this);
assert lock.isHeldByCurrentThread();
ExtLogRecord last = q.peekLast();
if (last != null) {
// check the ordering
Expand All @@ -185,7 +188,7 @@ private void enqueueOrdered(Deque<ExtLogRecord> q, ExtLogRecord record) {
}

private Supplier<ExtLogRecord> drain() {
assert Thread.holdsLock(this);
assert lock.isHeldByCurrentThread();
if (queues.isEmpty()) {
return () -> null;
}
Expand Down Expand Up @@ -233,21 +236,24 @@ private int compareSeq(ExtLogRecord min, ExtLogRecord testItem) {
@Override
public final void close() throws SecurityException {
checkAccess();
synchronized (this) {
lock.lock();
try {
if (!queues.isEmpty()) {
Formatter formatter = getFormatter();
if (formatter == null) {
formatter = new PatternFormatter("%d{yyyy-MM-dd HH:mm:ss,SSS} %-5p [%c] (%t) %s%e%n");
}
StandardOutputStreams.printError("The DelayedHandler was closed before any children handlers were " +
"configured. Messages will be written to stderr.");
"configured. Messages will be written to stderr.");
// Always attempt to drain the queue
Supplier<ExtLogRecord> drain = drain();
ExtLogRecord record;
while ((record = drain.get()) != null) {
StandardOutputStreams.printError(formatter.format(record));
}
}
} finally {
lock.unlock();
}
activated = false;
super.close();
Expand Down Expand Up @@ -353,16 +359,21 @@ public final boolean isActivated() {
return activated;
}

private synchronized void activate() {
// Always attempt to drain the queue
ExtLogRecord record;
final LogContext logContext = this.logContext;
Supplier<ExtLogRecord> drain = drain();
while ((record = drain.get()) != null) {
if (isEnabled() && isLoggable(record) && (logContext == null || logContext.getLogger(record.getLoggerName()).isLoggable(record.getLevel()))) {
publishToNestedHandlers(record);
private void activate() {
lock.lock();
try {
// Always attempt to drain the queue
ExtLogRecord record;
final LogContext logContext = this.logContext;
Supplier<ExtLogRecord> drain = drain();
while ((record = drain.get()) != null) {
if (isEnabled() && isLoggable(record) && (logContext == null || logContext.getLogger(record.getLoggerName()).isLoggable(record.getLevel()))) {
publishToNestedHandlers(record);
}
}
activated = true;
} finally {
lock.unlock();
}
activated = true;
}
}
Loading

0 comments on commit e992878

Please sign in to comment.