From cc2b443150c95501c5261b8dee2203705efc1dc1 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 26 Mar 2021 18:44:13 +0100 Subject: [PATCH] DefaultThreadFactory must not use Thread.currentThread() when constructed without ThreadGroup (#11119) Motivation: We had a bug in out DefaulThreadFactory as it always retrieved the ThreadGroup to used during creation time when now explicit ThreadGroup was given. This is problematic as the Thread may die and so the ThreadGroup is destroyed even tho the DefaultThreadFactory is still used. This could produce exceptions like: java.lang.IllegalThreadStateException at java.lang.ThreadGroup.addUnstarted(ThreadGroup.java:867) at java.lang.Thread.init(Thread.java:405) at java.lang.Thread.init(Thread.java:349) at java.lang.Thread.(Thread.java:599) at io.netty.util.concurrent.FastThreadLocalThread.(FastThreadLocalThread.java:60) at io.netty.util.concurrent.DefaultThreadFactory.newThread(DefaultThreadFactory.java:122) at io.netty.util.concurrent.DefaultThreadFactory.newThread(DefaultThreadFactory.java:106) at io.netty.util.concurrent.ThreadPerTaskExecutor.execute(ThreadPerTaskExecutor.java:32) at io.netty.util.internal.ThreadExecutorMap$1.execute(ThreadExecutorMap.java:57) at io.netty.util.concurrent.SingleThreadEventExecutor.doStartThread(SingleThreadEventExecutor.java:978) at io.netty.util.concurrent.SingleThreadEventExecutor.startThread(SingleThreadEventExecutor.java:947) at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:830) at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:818) at io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:471) at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:87) at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:81) at io.netty.channel.MultithreadEventLoopGroup.register(MultithreadEventLoopGroup.java:86) at io.netty.bootstrap.AbstractBootstrap.initAndRegister(AbstractBootstrap.java:323) at io.netty.bootstrap.AbstractBootstrap.doBind(AbstractBootstrap.java:272) at io.netty.bootstrap.AbstractBootstrap.bind(AbstractBootstrap.java:239) at io.netty.incubator.codec.quic.QuicTestUtils.newServer(QuicTestUtils.java:138) at io.netty.incubator.codec.quic.QuicTestUtils.newServer(QuicTestUtils.java:143) at io.netty.incubator.codec.quic.QuicTestUtils.newServer(QuicTestUtils.java:147) at io.netty.incubator.codec.quic.QuicStreamFrameTest.testCloseHalfClosure(QuicStreamFrameTest.java:48) at io.netty.incubator.codec.quic.QuicStreamFrameTest.testCloseHalfClosureUnidirectional(QuicStreamFrameTest.java:35) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:288) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:282) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.lang.Thread.run(Thread.java:748) Modifications: - If the user dont specify a ThreadGroup we will just pass null to the constructor of FastThreadLocalThread and so let it retrieve on creation time - Adjust tests Result: Don't risk to see IllegalThreadStateExceptions. --- .../util/concurrent/DefaultThreadFactory.java | 3 +- .../concurrent/DefaultThreadFactoryTest.java | 63 +++++++++++-------- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java b/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java index 3671cd7764..a18f236309 100644 --- a/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java +++ b/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java @@ -97,8 +97,7 @@ public class DefaultThreadFactory implements ThreadFactory { } public DefaultThreadFactory(String poolName, boolean daemon, int priority) { - this(poolName, daemon, priority, System.getSecurityManager() == null ? - Thread.currentThread().getThreadGroup() : System.getSecurityManager().getThreadGroup()); + this(poolName, daemon, priority, null); } @Override diff --git a/common/src/test/java/io/netty/util/concurrent/DefaultThreadFactoryTest.java b/common/src/test/java/io/netty/util/concurrent/DefaultThreadFactoryTest.java index ea04a38403..b338a8fded 100644 --- a/common/src/test/java/io/netty/util/concurrent/DefaultThreadFactoryTest.java +++ b/common/src/test/java/io/netty/util/concurrent/DefaultThreadFactoryTest.java @@ -24,7 +24,9 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; public class DefaultThreadFactoryTest { @Test(timeout = 2000) @@ -128,32 +130,6 @@ public class DefaultThreadFactoryTest { sticky); } - // test that when DefaultThreadFactory is constructed it is sticky to the thread group from the thread group of the - // thread that created it - @Test(timeout = 2000) - public void testDefaultThreadFactoryInheritsThreadGroup() throws InterruptedException { - final ThreadGroup sticky = new ThreadGroup("sticky"); - - runStickyThreadGroupTest( - new Callable() { - @Override - public DefaultThreadFactory call() throws Exception { - final AtomicReference factory = - new AtomicReference(); - final Thread thread = new Thread(sticky, new Runnable() { - @Override - public void run() { - factory.set(new DefaultThreadFactory("test")); - } - }); - thread.start(); - thread.join(); - return factory.get(); - } - }, - sticky); - } - // test that when a security manager is installed that provides a ThreadGroup, DefaultThreadFactory inherits from // the security manager @Test(timeout = 2000) @@ -263,4 +239,39 @@ public class DefaultThreadFactoryTest { assertEquals(secondGroup, secondCaptured.get()); } + + // test that when DefaultThreadFactory is constructed without a sticky thread group, threads + // created by it inherit the correct thread group + @Test(timeout = 2000) + public void testCurrentThreadGroupIsUsed() throws InterruptedException { + final AtomicReference factory = new AtomicReference(); + final AtomicReference firstCaptured = new AtomicReference(); + + final ThreadGroup group = new ThreadGroup("first"); + assertFalse(group.isDestroyed()); + final Thread first = new Thread(group, new Runnable() { + @Override + public void run() { + final Thread current = Thread.currentThread(); + firstCaptured.set(current.getThreadGroup()); + factory.set(new DefaultThreadFactory("sticky", false)); + } + }); + first.start(); + first.join(); + // Destroy the group now + group.destroy(); + assertTrue(group.isDestroyed()); + assertEquals(group, firstCaptured.get()); + + ThreadGroup currentThreadGroup = Thread.currentThread().getThreadGroup(); + Thread second = factory.get().newThread(new Runnable() { + @Override + public void run() { + // NOOP. + } + }); + second.join(); + assertEquals(currentThreadGroup, currentThreadGroup); + } }