Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust the Greengrass Discovery client to use its own executor service #427

Merged
merged 7 commits into from
Jun 15, 2023

Conversation

TwistedTwigleg
Copy link
Contributor

@TwistedTwigleg TwistedTwigleg commented Jun 6, 2023

Issue #, if available:

Fixes #320

Description of changes:

As part of #426, I was investigating why I was getting warnings saying threads were not being closed and it was resulting in a roughly minute long delay at the end of the sample, though the sample itself works fine and all the code executes.

The warning I got with the sample running locally in #426 was:

[WARNING] thread Thread[#33,ForkJoinPool.commonPool-worker-1,5,greengrass.BasicDiscovery] was interrupted but is still alive after waiting at least 15000msecs
[WARNING] thread Thread[#33,ForkJoinPool.commonPool-worker-1,5,greengrass.BasicDiscovery] will linger despite being asked to die via interruption
[WARNING] thread Thread[#34,ForkJoinPool.commonPool-worker-2,5,greengrass.BasicDiscovery] will linger despite being asked to die via interruption
[WARNING] NOTE: 2 thread(s) did not finish despite being asked to  via interruption. This is not a problem with exec:java, it is a problem with the running code. Although not serious, it should be remedied.

Which was a bit concerning as it seemed some thread was lingering after the sample was finished.

After a bunch of researching, poking around the code, and looking around, I concluded the issue seemed to be due to the common thread pool being used and not being properly cleaned up by default because it uses daemon/non-daemon threads and using the wrong type can lead the JVM to finish before the thread, orphaning the thread (if my admitted limited understanding is correct).
By default, supplyAsync() uses the default common thread pool, which has a tendency to have this issue to occur, apparently from what I gathered. While I didn't find an exact case that was showing the warnings I was seeing or was like our code with supplyAsync, the overall common advice I found while researching is to make and manage a thread pool, calling shutdown when finished, which frees the threads in question.

I made the adjustment in this PR, making the code manage its own single thread pool just for this single purpose. It also adds a new configuration option to set the executor manually, if desired, in the discovery client config. If set manually, the customer is expected to shut the executor down when finished. (this is also documented)

I tested and with this change, the minute long delay and warnings were gone! Everything still functions properly too.

Looking at the issue in #320, it seemed similar and so I tested it, and it seems to work there now too! I confirmed that without the change the issue occurs and you get an exception, and with the change, the issue is not present. Based on really quick research into that issue, it seems that the default common thread pool does NOT carry over the permissions of the executing Jar, but if you make one manually, it does properly inherit the security permissions.

TLDR: The delay that I was seeing in #426 is fixed by having the discovery client make and destroy its own thread pool in an executor service, and it happened to fix the SecurityManager issue as well.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sbSteveK sbSteveK merged commit 242e97d into main Jun 15, 2023
@sbSteveK sbSteveK deleted the greengrass_discovery_thread_fix branch June 15, 2023 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Greengrass DiscoveryClient does not work if a SecurityManager is installed
4 participants