xiaoyuyao commented on a change in pull request #2828:
URL: https://github.com/apache/hadoop/pull/2828#discussion_r603483652
##########
File path:
hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMS.java
##########
@@ -550,7 +550,7 @@ public Void call() throws Exception {
threadGroup.enumerate(threads);
Review comment:
Thanks @aajisaka for reporting the issue and the fix.
I think the problem is Line 550: ThreadGroup#enumerate does not provide a
reliable way to get all the threads based on the JDK document. This is
problematic especially if the threads array is not big enough to hold all the
threads returned, the extra will be silently skipped. In the case of the
reloader thread is skipped, we will have a reloaderThead as null for NPE. The
proposed fix may not completely solve the problem.
I would suggest we use Apache common ThreadUtils.findThreadsByName to
replace the code Line 547-563, which simplify the code with better handling of
this case to avoid NPE.
```suggestion
Collection<Thread> result =
ThreadUtils.findThreadsByName(SSL_RELOADER_THREAD_NAME);
Assert.assertEquals(1, result.size());
Assert.assertTrue("Reloader is not alive",
result.iterator().next().isAlive());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
