DefaultThreadFactory must not use Thread.currentThread() when constru…

…cted 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.<init>(Thread.java:599)
        at io.netty.util.concurrent.FastThreadLocalThread.<init>(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.

common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java

@@ -97,8 +97,7 @@ public DefaultThreadFactory(String poolName, boolean daemon, int priority, Threa
     }
 
     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

common/src/test/java/io/netty/util/concurrent/DefaultThreadFactoryTest.java

@@ -24,7 +24,9 @@
 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 DefaultThreadFactory call() throws Exception {
                 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<DefaultThreadFactory>() {
-                    @Override
-                    public DefaultThreadFactory call() throws Exception {
-                        final AtomicReference<DefaultThreadFactory> factory =
-                                new AtomicReference<DefaultThreadFactory>();
-                        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 void run() {
 
         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<DefaultThreadFactory> factory = new AtomicReference<DefaultThreadFactory>();
+        final AtomicReference<ThreadGroup> firstCaptured = new AtomicReference<ThreadGroup>();
+
+        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);
+    }
 }