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

Do not leak running pools from the internal collection #3885

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

joerg1985
Copy link
Contributor

Context

In theory the ThreadPoolManager could leak running thread pools an create new ones.
I have not seen this in the wild, but it could happen.

The ThreadPoolManager did use a WeakHashMap<String, ExecutorService> to store the running pools.
It holds a weak reference to the String and a strong to the ExecutorService.
As soon as the String gets collected by the GC, the strong reference to the ExecutorService is dopped.

This is a example of problematic code, this could create two pools when GC did clear the WeakReferences between the two calls to ThreadPoolManager.getScheduledPoolUnwrapped:

ThreadPoolManager.getScheduledPoolUnwrapped("test" + 1);
System.gc();
ThreadPoolManager.getScheduledPoolUnwrapped("test" + 1);

Beside this, there are other issues like accessing the WeakHashMap outside a synchronized block.
This double check logic used is in my mind only safe for immutable objects...

Description

This PR will replace the WeakHashMap with a ConcurrentHashMap to fix the issues above.

@joerg1985 joerg1985 requested a review from a team as a code owner November 24, 2023 20:17
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
It feels as if the use of WeakHashMap didn't make any sense here in the first place, right? I assume the thinking was rather that the value is a week reference and that the entry should be removed from the map in case that the pool is not used anywhere anymore. But having weak references to String keys is just wrong.

@kaikreuzer kaikreuzer merged commit 32237a9 into openhab:main Nov 24, 2023
3 checks passed
@kaikreuzer kaikreuzer added the bug An unexpected problem or unintended behavior of the Core label Nov 24, 2023
@kaikreuzer kaikreuzer added this to the 4.1 milestone Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants