andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

DefaultThreadFactory must not use Thread.currentThread() when constructed without ThreadGroup (#11119)

Browse Source 
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.
This commit is contained in:
Norman Maurer 2021-03-26 18:44:13 +01:00
parent aa1b887536
commit 50a83ec8a8
2 changed files with 32 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java
View File

@ -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

49
common/src/test/java/io/netty/util/concurrent/DefaultThreadFactoryTest.java
View File

@ -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)
@ -112,24 +114,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(
() -> {
final AtomicReference<DefaultThreadFactory> factory =
new AtomicReference<>();
final Thread thread = new Thread(sticky, () -> 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)
@ -216,4 +200,33 @@ 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<DefaultThreadFactory> factory = new AtomicReference<>();
final AtomicReference<ThreadGroup> firstCaptured = new AtomicReference<>();
final ThreadGroup group = new ThreadGroup("first");
assertFalse(group.isDestroyed());
final Thread first = new Thread(group, () -> {
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(() -> {
// NOOP.
});
second.join();
assertEquals(currentThreadGroup, currentThreadGroup);
}
}