From 1d08ca8ef534ba1deec5bca0e04ab2df1ad56c37 Mon Sep 17 00:00:00 2001 From: James Perkins Date: Mon, 7 Oct 2013 15:11:25 -0700 Subject: [PATCH] [Bugzilla 176] Make the LoggerFactory initialization state thread safe. Test taken from attachment on the bugzilla, http://bugzilla.slf4j.org/show_bug.cgi?id=176. --- .../src/main/java/org/slf4j/LoggerFactory.java | 34 ++++++--- .../test/java/org/slf4j/ConcurrentInitTest.java | 81 ++++++++++++++++++++++ 2 files changed, 106 insertions(+), 9 deletions(-) create mode 100644 slf4j-api/src/test/java/org/slf4j/ConcurrentInitTest.java diff --git a/slf4j-api/src/main/java/org/slf4j/LoggerFactory.java b/slf4j-api/src/main/java/org/slf4j/LoggerFactory.java index 3994188..c8a3f8b 100644 --- a/slf4j-api/src/main/java/org/slf4j/LoggerFactory.java +++ b/slf4j-api/src/main/java/org/slf4j/LoggerFactory.java @@ -70,9 +70,13 @@ public final class LoggerFactory { static final int SUCCESSFUL_INITIALIZATION = 3; static final int NOP_FALLBACK_INITIALIZATION = 4; - static int INITIALIZATION_STATE = UNINITIALIZED; - static SubstituteLoggerFactory TEMP_FACTORY = new SubstituteLoggerFactory(); - static NOPLoggerFactory NOP_FALLBACK_FACTORY = new NOPLoggerFactory(); + static volatile int INITIALIZATION_STATE = UNINITIALIZED; + static volatile SubstituteLoggerFactory TEMP_FACTORY = new SubstituteLoggerFactory(); + static volatile NOPLoggerFactory NOP_FALLBACK_FACTORY = new NOPLoggerFactory(); + /** + * Used as a lock for read/writing state of {@link #INITIALIZATION_STATE}. + */ + static final Object STATE_LOCK = new Object(); /** * It is LoggerFactory's responsibility to track version changes and manage @@ -99,7 +103,9 @@ public final class LoggerFactory { * You are strongly discouraged from calling this method in production code. */ static void reset() { - INITIALIZATION_STATE = UNINITIALIZED; + synchronized (STATE_LOCK) { + INITIALIZATION_STATE = UNINITIALIZED; + } TEMP_FACTORY = new SubstituteLoggerFactory(); } @@ -157,7 +163,9 @@ public final class LoggerFactory { } static void failedBinding(Throwable t) { - INITIALIZATION_STATE = FAILED_INITIALIZATION; + synchronized (STATE_LOCK) { + INITIALIZATION_STATE = FAILED_INITIALIZATION; + } Util.report("Failed to instantiate SLF4J LoggerFactory", t); } @@ -290,11 +298,19 @@ public final class LoggerFactory { * @return the ILoggerFactory instance in use */ public static ILoggerFactory getILoggerFactory() { - if (INITIALIZATION_STATE == UNINITIALIZED) { - INITIALIZATION_STATE = ONGOING_INITIALIZATION; - performInitialization(); + // If re-entrant, return the temp factory + if (Thread.holdsLock(STATE_LOCK) && INITIALIZATION_STATE == ONGOING_INITIALIZATION) { + return TEMP_FACTORY; + } + final int currentState; + synchronized (STATE_LOCK) { + if (INITIALIZATION_STATE == UNINITIALIZED) { + INITIALIZATION_STATE = ONGOING_INITIALIZATION; + performInitialization(); + } + currentState = INITIALIZATION_STATE; } - switch (INITIALIZATION_STATE) { + switch (currentState) { case SUCCESSFUL_INITIALIZATION: return StaticLoggerBinder.getSingleton().getLoggerFactory(); case NOP_FALLBACK_INITIALIZATION: diff --git a/slf4j-api/src/test/java/org/slf4j/ConcurrentInitTest.java b/slf4j-api/src/test/java/org/slf4j/ConcurrentInitTest.java new file mode 100644 index 0000000..3ad7653 --- /dev/null +++ b/slf4j-api/src/test/java/org/slf4j/ConcurrentInitTest.java @@ -0,0 +1,81 @@ +/** + * Copyright 2013 Simon Arlott + * + * Permission is hereby granted, free of charge, to any person obtaining + * a copy of this software and associated documentation files (the + * "Software"), to deal in the Software without restriction, including + * without limitation the rights to use, copy, modify, merge, publish, + * distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to + * the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE + * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION + * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ + +package org.slf4j; + +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.helpers.NOPLoggerFactory; +import org.slf4j.helpers.SubstituteLoggerFactory; + +import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.Callable; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.FutureTask; + +public class ConcurrentInitTest { + private static class GetLogger implements Callable { + private CyclicBarrier barrier; + + public GetLogger(CyclicBarrier barrier) { + this.barrier = barrier; + } + + public ILoggerFactory call() throws InterruptedException, BrokenBarrierException { + barrier.await(); + return LoggerFactory.getILoggerFactory(); + } + } + + @Test + public void getLogger() throws InterruptedException, BrokenBarrierException, ExecutionException { + // Create a lot of threads that will all request the logger factory + @SuppressWarnings("unchecked") + FutureTask[] tasks = new FutureTask[10 + Runtime.getRuntime().availableProcessors() * 2]; + CyclicBarrier barrier = new CyclicBarrier(tasks.length + 1); + try { + for (int i = 0; i < tasks.length; i++) { + tasks[i] = new FutureTask(new GetLogger(barrier)); + + // Each thread will block on the barrier + new Thread(tasks[i]).start(); + } + + // Wait for all threads to be at the barrier and allow them to run + // all at the same time + barrier.await(); + } finally { + barrier.reset(); + } + + for (int i = 0; i < tasks.length; i++) { + // If any of the logger factories are SubstituteLoggerFactory then + // initialisation failed + Assert.assertFalse(i + "=" + tasks[i].get().getClass().getName(), + tasks[i].get() instanceof SubstituteLoggerFactory); + Assert.assertTrue(i + "=" + tasks[i].get().getClass().getName(), + tasks[i].get() instanceof NOPLoggerFactory); + } + } +} -- 1.8.3.1