From 31da0ddbac1a07e6efa5e50334e09955b94ae62b Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 21 Dec 2016 15:14:53 +0100 Subject: [PATCH] Revert "Disallow setting logger factory twice" This reverts commit 3c92f2b64a1c6803860ab310348abc9f1cb47ff4 which needs more thoughts and so will go into the next release. --- .../logging/InternalLoggerFactory.java | 91 +++---- .../logging/InternalLoggerFactoryTest.java | 229 +++--------------- 2 files changed, 55 insertions(+), 265 deletions(-) diff --git a/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java b/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java index 2b19d5c026..9f85e3646b 100644 --- a/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java +++ b/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java @@ -16,64 +16,24 @@ package io.netty.util.internal.logging; -import java.util.concurrent.atomic.AtomicReference; - /** - * Creates {@link InternalLogger}s. This factory allows you to choose what logging framework Netty should use. The - * default factory is {@link Slf4JLoggerFactory}. If SLF4J is not available, {@link Log4JLoggerFactory} is used. If - * Log4J is not available, {@link JdkLoggerFactory} is used. You can change it to your preferred logging framework - * before other Netty classes are loaded via {@link #setDefaultFactory(InternalLoggerFactory)}. If you want to change - * the logger factory, {@link #setDefaultFactory(InternalLoggerFactory)} must be invoked before any other Netty classes - * are loaded. Note that {@link #setDefaultFactory(InternalLoggerFactory)}} can not be invoked more than once. + * Creates an {@link InternalLogger} or changes the default factory + * implementation. This factory allows you to choose what logging framework + * Netty should use. The default factory is {@link Slf4JLoggerFactory}. If SLF4J + * is not available, {@link Log4JLoggerFactory} is used. If Log4J is not available, + * {@link JdkLoggerFactory} is used. You can change it to your preferred + * logging framework before other Netty classes are loaded: + * 
+ * {@link InternalLoggerFactory}.setDefaultFactory({@link Log4JLoggerFactory}.INSTANCE);
+ *
+ * Please note that the new default factory is effective only for the classes + * which were loaded after the default factory is changed. Therefore, + * {@link #setDefaultFactory(InternalLoggerFactory)} should be called as early + * as possible and shouldn't be called more than once. */ public abstract class InternalLoggerFactory { - private static final InternalLoggerFactoryHolder HOLDER = new InternalLoggerFactoryHolder(); - - /** - * This class holds a reference to the {@link InternalLoggerFactory}. The raison d'être for this class is primarily - * to aid in testing. - */ - static final class InternalLoggerFactoryHolder { - private final AtomicReference reference; - - InternalLoggerFactoryHolder() { - this(null); - } - - InternalLoggerFactoryHolder(final InternalLoggerFactory holder) { - this.reference = new AtomicReference(holder); - } - - InternalLoggerFactory getFactory() { - if (reference.get() == null) { - reference.compareAndSet(null, newDefaultFactory(InternalLoggerFactory.class.getName())); - } - return reference.get(); - } - - void setFactory(final InternalLoggerFactory factory) { - if (factory == null) { - throw new NullPointerException("factory"); - } - if (!reference.compareAndSet(null, factory)) { - throw new IllegalStateException( - "factory is already set to [" + reference.get() + "], rejecting [" + factory + "]"); - } - } - - InternalLogger getInstance(final Class clazz) { - return getInstance(clazz.getName()); - } - - InternalLogger getInstance(final String name) { - return newInstance(name); - } - - InternalLogger newInstance(String name) { - return getFactory().newInstance(name); - } - } + private static volatile InternalLoggerFactory defaultFactory; @SuppressWarnings("UnusedCatchParameter") private static InternalLoggerFactory newDefaultFactory(String name) { @@ -94,35 +54,38 @@ public abstract class InternalLoggerFactory { } /** - * Get the default factory that was either initialized automatically based on logging implementations on the - * classpath, or set explicitly via {@link #setDefaultFactory(InternalLoggerFactory)}. + * Returns the default factory. The initial default factory is + * {@link JdkLoggerFactory}. */ public static InternalLoggerFactory getDefaultFactory() { - return HOLDER.getFactory(); + if (defaultFactory == null) { + defaultFactory = newDefaultFactory(InternalLoggerFactory.class.getName()); + } + return defaultFactory; } /** - * Set the default factory. This method must be invoked before the default factory is initialized via - * {@link #getDefaultFactory()}, and can not be invoked multiple times. - * - * @param defaultFactory a non-null implementation of {@link InternalLoggerFactory} + * Changes the default factory. */ public static void setDefaultFactory(InternalLoggerFactory defaultFactory) { - HOLDER.setFactory(defaultFactory); + if (defaultFactory == null) { + throw new NullPointerException("defaultFactory"); + } + InternalLoggerFactory.defaultFactory = defaultFactory; } /** * Creates a new logger instance with the name of the specified class. */ public static InternalLogger getInstance(Class clazz) { - return HOLDER.getInstance(clazz); + return getInstance(clazz.getName()); } /** * Creates a new logger instance with the specified name. */ public static InternalLogger getInstance(String name) { - return HOLDER.getInstance(name); + return getDefaultFactory().newInstance(name); } /** diff --git a/common/src/test/java/io/netty/util/internal/logging/InternalLoggerFactoryTest.java b/common/src/test/java/io/netty/util/internal/logging/InternalLoggerFactoryTest.java index d040943ccf..0a9f85a3f7 100644 --- a/common/src/test/java/io/netty/util/internal/logging/InternalLoggerFactoryTest.java +++ b/common/src/test/java/io/netty/util/internal/logging/InternalLoggerFactoryTest.java @@ -13,63 +13,49 @@ * License for the specific language governing permissions and limitations * under the License. */ - package io.netty.util.internal.logging; import org.junit.After; import org.junit.Before; import org.junit.Test; -import java.util.concurrent.BrokenBarrierException; -import java.util.concurrent.CyclicBarrier; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicReference; - -import static org.easymock.EasyMock.createMock; -import static org.easymock.EasyMock.createStrictMock; -import static org.easymock.EasyMock.expect; -import static org.easymock.EasyMock.replay; -import static org.easymock.EasyMock.reset; -import static org.easymock.EasyMock.verify; -import static org.hamcrest.CoreMatchers.containsString; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNotSame; -import static org.junit.Assert.assertSame; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.easymock.EasyMock.*; +import static org.junit.Assert.*; public class InternalLoggerFactoryTest { private static final Exception e = new Exception(); - private InternalLoggerFactory.InternalLoggerFactoryHolder holder; + private InternalLoggerFactory oldLoggerFactory; private InternalLogger mock; @Before public void init() { - final InternalLoggerFactory mockFactory = createMock(InternalLoggerFactory.class); + oldLoggerFactory = InternalLoggerFactory.getDefaultFactory(); + InternalLoggerFactory mockFactory = createMock(InternalLoggerFactory.class); mock = createStrictMock(InternalLogger.class); expect(mockFactory.newInstance("mock")).andReturn(mock).anyTimes(); replay(mockFactory); - holder = new InternalLoggerFactory.InternalLoggerFactoryHolder(mockFactory); + InternalLoggerFactory.setDefaultFactory(mockFactory); } @After public void destroy() { reset(mock); + InternalLoggerFactory.setDefaultFactory(oldLoggerFactory); } @Test(expected = NullPointerException.class) public void shouldNotAllowNullDefaultFactory() { - holder.setFactory(null); + InternalLoggerFactory.setDefaultFactory(null); } @Test public void shouldGetInstance() { - final String helloWorld = "Hello, world!"; + InternalLoggerFactory.setDefaultFactory(oldLoggerFactory); - final InternalLogger one = InternalLoggerFactory.getInstance("helloWorld"); - final InternalLogger two = InternalLoggerFactory.getInstance(helloWorld.getClass()); + String helloWorld = "Hello, world!"; + + InternalLogger one = InternalLoggerFactory.getInstance("helloWorld"); + InternalLogger two = InternalLoggerFactory.getInstance(helloWorld.getClass()); assertNotNull(one); assertNotNull(two); @@ -81,7 +67,7 @@ public class InternalLoggerFactoryTest { expect(mock.isTraceEnabled()).andReturn(true); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); assertTrue(logger.isTraceEnabled()); verify(mock); } @@ -91,7 +77,7 @@ public class InternalLoggerFactoryTest { expect(mock.isDebugEnabled()).andReturn(true); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); assertTrue(logger.isDebugEnabled()); verify(mock); } @@ -101,7 +87,7 @@ public class InternalLoggerFactoryTest { expect(mock.isInfoEnabled()).andReturn(true); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); assertTrue(logger.isInfoEnabled()); verify(mock); } @@ -111,7 +97,7 @@ public class InternalLoggerFactoryTest { expect(mock.isWarnEnabled()).andReturn(true); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); assertTrue(logger.isWarnEnabled()); verify(mock); } @@ -121,7 +107,7 @@ public class InternalLoggerFactoryTest { expect(mock.isErrorEnabled()).andReturn(true); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); assertTrue(logger.isErrorEnabled()); verify(mock); } @@ -131,7 +117,7 @@ public class InternalLoggerFactoryTest { mock.trace("a"); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.trace("a"); verify(mock); } @@ -141,7 +127,7 @@ public class InternalLoggerFactoryTest { mock.trace("a", e); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.trace("a", e); verify(mock); } @@ -151,7 +137,7 @@ public class InternalLoggerFactoryTest { mock.debug("a"); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.debug("a"); verify(mock); } @@ -161,7 +147,7 @@ public class InternalLoggerFactoryTest { mock.debug("a", e); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.debug("a", e); verify(mock); } @@ -171,7 +157,7 @@ public class InternalLoggerFactoryTest { mock.info("a"); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.info("a"); verify(mock); } @@ -181,7 +167,7 @@ public class InternalLoggerFactoryTest { mock.info("a", e); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.info("a", e); verify(mock); } @@ -191,7 +177,7 @@ public class InternalLoggerFactoryTest { mock.warn("a"); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.warn("a"); verify(mock); } @@ -201,7 +187,7 @@ public class InternalLoggerFactoryTest { mock.warn("a", e); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.warn("a", e); verify(mock); } @@ -211,7 +197,7 @@ public class InternalLoggerFactoryTest { mock.error("a"); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.error("a"); verify(mock); } @@ -221,167 +207,8 @@ public class InternalLoggerFactoryTest { mock.error("a", e); replay(mock); - final InternalLogger logger = holder.getInstance("mock"); + InternalLogger logger = InternalLoggerFactory.getInstance("mock"); logger.error("a", e); verify(mock); } - - @Test - public void shouldNotAllowToSetFactoryTwice() { - try { - holder.setFactory(createMock(InternalLoggerFactory.class)); - fail("should have thrown IllegalStateException"); - } catch (final IllegalStateException e) { - assertThat(e.getMessage(), containsString("factory is already set")); - } - - try { - final InternalLoggerFactory.InternalLoggerFactoryHolder implicit = - new InternalLoggerFactory.InternalLoggerFactoryHolder(); - implicit.getFactory(); // force initialization - implicit.setFactory(createMock(InternalLoggerFactory.class)); - fail("should have thrown IllegalStateException"); - } catch (final IllegalStateException e) { - assertThat(e.getMessage(), containsString("factory is already set")); - } - } - - @Test - public void raceGetAndGet() throws BrokenBarrierException, InterruptedException { - final CyclicBarrier barrier = new CyclicBarrier(3); - final InternalLoggerFactory.InternalLoggerFactoryHolder holder = - new InternalLoggerFactory.InternalLoggerFactoryHolder(); - final AtomicReference firstReference = new AtomicReference(); - final AtomicReference secondReference = new AtomicReference(); - - final Thread firstGet = getThread(firstReference, holder, barrier); - final Thread secondGet = getThread(secondReference, holder, barrier); - - firstGet.start(); - secondGet.start(); - // start the two get threads - barrier.await(); - - // wait for the two get threads to complete - barrier.await(); - - if (holder.getFactory() == firstReference.get()) { - assertSame(holder.getFactory(), secondReference.get()); - } else if (holder.getFactory() == secondReference.get()) { - assertSame(holder.getFactory(), firstReference.get()); - } else { - fail("holder should have been set by one of the get threads"); - } - } - - @Test - public void raceGetAndSet() throws BrokenBarrierException, InterruptedException { - final CyclicBarrier barrier = new CyclicBarrier(3); - final InternalLoggerFactory.InternalLoggerFactoryHolder holder = - new InternalLoggerFactory.InternalLoggerFactoryHolder(); - final InternalLoggerFactory internalLoggerFactory = createMock(InternalLoggerFactory.class); - final AtomicReference reference = new AtomicReference(); - - final Thread get = getThread(reference, holder, barrier); - - final AtomicBoolean setSuccess = new AtomicBoolean(); - final Thread set = setThread(internalLoggerFactory, holder, setSuccess, barrier); - - get.start(); - set.start(); - // start the get and set threads - barrier.await(); - - // wait for the get and set threads to complete - barrier.await(); - - if (setSuccess.get()) { - assertSame(internalLoggerFactory, reference.get()); - assertSame(internalLoggerFactory, holder.getFactory()); - } else { - assertNotSame(internalLoggerFactory, reference.get()); - assertNotSame(internalLoggerFactory, holder.getFactory()); - assertSame(holder.getFactory(), reference.get()); - } - } - - @Test - public void raceSetAndSet() throws BrokenBarrierException, InterruptedException { - final CyclicBarrier barrier = new CyclicBarrier(3); - final InternalLoggerFactory.InternalLoggerFactoryHolder holder = - new InternalLoggerFactory.InternalLoggerFactoryHolder(); - final InternalLoggerFactory first = createMock(InternalLoggerFactory.class); - final InternalLoggerFactory second = createMock(InternalLoggerFactory.class); - - final AtomicBoolean firstSetSuccess = new AtomicBoolean(); - final Thread firstSet = setThread(first, holder, firstSetSuccess, barrier); - - final AtomicBoolean secondSetSuccess = new AtomicBoolean(); - final Thread secondSet = setThread(second, holder, secondSetSuccess, barrier); - - firstSet.start(); - secondSet.start(); - // start the two set threads - barrier.await(); - - // wait for the two set threads to complete - barrier.await(); - - assertTrue(firstSetSuccess.get() || secondSetSuccess.get()); - if (firstSetSuccess.get()) { - assertFalse(secondSetSuccess.get()); - assertSame(first, holder.getFactory()); - } else { - assertFalse(firstSetSuccess.get()); - assertSame(second, holder.getFactory()); - } - } - - private static Thread getThread( - final AtomicReference reference, - final InternalLoggerFactory.InternalLoggerFactoryHolder holder, - final CyclicBarrier barrier) { - return new Thread(new Runnable() { - @Override - public void run() { - awaitUnchecked(barrier); - reference.set(holder.getFactory()); - awaitUnchecked(barrier); - } - }); - } - - private static Thread setThread( - final InternalLoggerFactory internalLoggerFactory, - final InternalLoggerFactory.InternalLoggerFactoryHolder holder, - final AtomicBoolean setSuccess, - final CyclicBarrier barrier) { - return new Thread(new Runnable() { - @Override - public void run() { - awaitUnchecked(barrier); - boolean success = true; - try { - holder.setFactory(internalLoggerFactory); - } catch (final IllegalStateException e) { - success = false; - assertThat(e.getMessage(), containsString("factory is already set")); - } finally { - setSuccess.set(success); - awaitUnchecked(barrier); - } - } - }); - } - - private static void awaitUnchecked(final CyclicBarrier barrier) { - try { - barrier.await(); - } catch (final InterruptedException exception) { - throw new IllegalStateException(exception); - } catch (final BrokenBarrierException exception) { - throw new IllegalStateException(exception); - } - } - }