Repository: netty/netty
Issue: DefaultThreadFactory should stick to the thread group where it was constructed
Labels: defect

We (gRPC) encountered a bug that was triggered by https://github.com/grpc/grpc-java/commit/d927180a63ac2df4fae0082b51d51e3836771bb1. After that commit, event loop threads are created per task by `NioEventLoopGroup`, and inherits the thread group of the caller, which in our case is an application-provided request-scope thread. Things go south when the application tries to manipulate (e.g., interrupt and join) all threads of the request-scope thread group, which unexpectedly include the event loop threads.

I think `DefaultThreadFactory` should save the current thread group in constructor, and apply it to all new threads. I will come up with a PR soon.

@buchgr


Issue comments:
@zhangkun83 sounds good. Thanks!

@zhangkun83 please ensure you sign our icla: http://netty.io/s/icla and use our commit message template http://netty.io/wiki/writing-a-commit-message.html when doing the pr :)

@zhangkun83 good catch!

The fix in this PR is a mistake. For just one example, it means that the `DefaultThreadFactory` field in `GlobalEventExecutor` will have its thread group forever tied to the thread group that triggered the initialization of the `GlobalEventExecutor`. Since the `GlobalEventExecutor` is a global, a thread from another thread group could trigger the creation of a thread from this factory and will modify properties on the created thread. This is completely counter to the point of thread groups, and certain security managers will slap this down. Same for the `ThreadDeathWatcher`. I will open a PR to change.

++ @jasontedor 

looking forward to a PR @jasontedor 

+1

@Scottmitch @normanmaurer It will be inbound later today, but it's basically going to amount to reverting the commit in this PR. I wanted to write a failing test case first to demonstrate the problem, and a test case to guard against future changes which I have up in a [gist](https://gist.github.com/jasontedor/7716d29c6bcc75df78b86341961dc6ca/665a2f07dc5d80b93e17d41a142d1c2b426d5541). I'll formalize this soon.

@zhangkun83 @buchgr @nmittler @ejona86 WDYT ?

I opened #5533.

It seems worse to have the threads be mixed from a random thread groups. If you create a thread from an event loop, then you could never predict what thread group it will be part of.

> will modify properties on the created thread.

Which properties are you referring to here? If it's name, I'd imagine we should just not care and ignore changing the name if it fails. It seems daemon would be the main issue here.

> This is completely counter to the point of thread groups

I don't think I understand this point. Would it make sense if they created their threads eagerly when created? I've already noticed that shutdown() creates any un-created threads anyway (just to then shut them down).

> It seems worse to have the threads be mixed from a random thread groups. If you create a thread from an event loop, then you could never predict what thread group it will be part of.

This is a problem brought on by global singletons like `GlobalEventExecutor`.

> Which properties are you referring to here? If it's name, I'd imagine we should just not care and ignore changing the name if it fails. It seems daemon would be the main issue here.

Setting daemon and priority, these both require permissions, and implicitly the uncaught exception handler will be inherited.

> Would it make sense if they created their threads eagerly when created?

I'm not sure if this makes sense in the context of, for example, `GlobalEventExecutor` or `ThreadDeathWatcher` which fire up a thread as needed.

So maybe we allow a `null` ThreadGroup, which would mean to use the current Thread's ThreadGroup, and then the singletons would pass `null`? It seems that most event loops should use a consistent ThreadGroup, but yes, it seems that we need to do something weird for the singletons. Maybe it is also necessary to add `setThreadFactory()` methods to the two singletons.

Sounds ok to me... Not sure we need the setters though

> Am 13.07.2016 um 22:43 schrieb Eric Anderson notifications@github.com:
> 
> So maybe we allow a null ThreadGroup, which would mean to use the current ThreadGroup, and then the singletons would pass null? It seems that most event loops should use a consistent ThreadGroup, but yes, it seems that we need to do something weird for the singletons. Maybe it is also necessary to add setThreadFactory() methods to the two singletons.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

> So maybe we allow a `null` ThreadGroup, which would mean to use the current Thread's ThreadGroup, and then the singletons would pass `null`?

This is roughly the approach that I took in #5533. The difference is I just make the constructors on `DefaultThreadGroup` that do not take a `ThreadGroup` pass `null` leading to the desired behavior for the singletons.

I don't care too much about the setters at the moment. But it would seem likely to be necessary eventually. Any time multiple ThreadGroups are actually in use "weird things" can happen if you don't use a consistent ThreadGroup from an event loop.

I'd want the default for DefaultThreadFactory to be a consistent and predictable ThreadGroup. Executors/ThreadPoolExecutor makes sure to use a consistent and predictable ThreadGroup decided at construction, only differing by trying System.getSecurityManager().getThreadGroup() first. Maybe that tweak is all that's necessary to fix the SecurityManager woes and there's no need to use the current Thread's ThreadGroup during newThread().

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/concurrent/Executors.java#596
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/util/concurrent/ThreadPoolExecutor.java#1195

> Executors/ThreadPoolExecutor makes sure to use a consistent and predictable ThreadGroup decided at construction only differing by trying System.getSecurityManager().getThreadGroup() first.

We take the same approach in [Elasticsearch](https://github.com/elastic/elasticsearch/blob/1bc12f52140d2e89d971b81b6d07c86192e655a1/core/src/main/java/org/elasticsearch/common/util/concurrent/EsExecutors.java#L118-L120). However, as this leads to a fixed thread group, the problem here in Netty is still the singletons like `GlobalEventExecutor`.

@jasontedor wouldn't it be "good enough" for the singletons to just pass "null" in ?

> wouldn't it be "good enough" for the singletons to just pass "null" in ?

@normanmaurer It's fine, but I suppose the problem I see there is that it requires changing a bunch of call sites to use the four-args constructor that takes a thread group, or adding even more convenience constructors to a class that already has a lot of constructors. I'm happy to evolve #5533 as needed.

@jasontedor I think I would be fine to change the call sites. No need to add even more constructors. There should be not too many singletons that need the factory.

> I think I would be fine to change the call sites. No need to add even more constructors. There should be not too many singletons that need the factory.

@normanmaurer Sounds good, I pushed a commit to #5533 to reflect this.


Pull request review comments:
@zhangkun83 could you add a "TODO:" here? Something like:

`// TODO: Once we can break the API we should add ThreadGroup to the arguments of this method.`

@zhangkun83 sorry for be a PITA but I just noticed we are missing a null check for threadGroup here.

Please do:
`this.threadGroup = ObjectUtil.checkNonNull(threadGroup);`

![INFO](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-info.png) Complete the task associated to this TODO comment. [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=squid%3AS1135)

thats fine

All right. Added the null check.


Patch that resolved the issue:
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 84d72cdf242..48966839566 100644
--- a/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java
+++ b/common/src/main/java/io/netty/util/concurrent/DefaultThreadFactory.java
@@ -16,6 +16,7 @@
 
 package io.netty.util.concurrent;
 
+import io.netty.util.internal.ObjectUtil;
 import io.netty.util.internal.StringUtil;
 
 import java.util.Locale;
@@ -33,6 +34,7 @@ public class DefaultThreadFactory implements ThreadFactory {
     private final String prefix;
     private final boolean daemon;
     private final int priority;
+    private final ThreadGroup threadGroup;
 
     public DefaultThreadFactory(Class<?> poolType) {
         this(poolType, false, Thread.NORM_PRIORITY);
@@ -82,7 +84,7 @@ private static String toPoolName(Class<?> poolType) {
         }
     }
 
-    public DefaultThreadFactory(String poolName, boolean daemon, int priority) {
+    public DefaultThreadFactory(String poolName, boolean daemon, int priority, ThreadGroup threadGroup) {
         if (poolName == null) {
             throw new NullPointerException("poolName");
         }
@@ -94,6 +96,11 @@ public DefaultThreadFactory(String poolName, boolean daemon, int priority) {
         prefix = poolName + '-' + poolId.incrementAndGet() + '-';
         this.daemon = daemon;
         this.priority = priority;
+        this.threadGroup = ObjectUtil.checkNotNull(threadGroup, "threadGroup is null");
+    }
+
+    public DefaultThreadFactory(String poolName, boolean daemon, int priority) {
+        this(poolName, daemon, priority, Thread.currentThread().getThreadGroup());
     }
 
     @Override
@@ -119,8 +126,9 @@ public Thread newThread(Runnable r) {
         return t;
     }
 
+    // TODO: Once we can break the API we should add ThreadGroup to the arguments of this method.
     protected Thread newThread(Runnable r, String name) {
-        return new FastThreadLocalThread(r, name);
+        return new FastThreadLocalThread(threadGroup, r, name);
     }
 
     private static final class DefaultRunnableDecorator implements Runnable {


Test patch: