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

Replace ThreadLocal with a hashtable to support reliable cleanup #588

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Replace ThreadLocal with a hashtable to support reliable cleanup #588

merged 2 commits into from
Jul 23, 2024

Conversation

WolfgangHG
Copy link
Contributor

This is another attempt to work around the ThreadLocal memory leak that happens in arquillian-extension-warp. The first pull request #501 fails on Java 11 and was reverted by #579 (issue #578).

I replaced ThreadLocal with a Hashtable<Long, T> inside a utility class, where the key is the thread id. This way, it is easy to cleanup the full cache, which is done in ManagerImpl and AbstractContext.

What do you think? Is this an approach that could work? Or do I have a basic misunderstanding about ThreadLocals or did I break something else?

At least I could run the arquillian-extension-warp testsuite against a remote WildFly server ten times without OutOfMemoryException, while it failed before after 5-7 runs.

@starksm64
Copy link
Member

starksm64 commented Jul 9, 2024

Since you have demonstrated a usecase where the thread id is not a valid key for the thrown exception set, it seems like what the concurrency requirements are around this nested exception handling need to be defined. As far as I know the arquillian core is not really designed to run multiple tests in parallel, so why this was keyed on threads vs just tracking the exception class is not clear.

@WolfgangHG
Copy link
Contributor Author

@starksm64 Is this a comment about my pull request or an overall philosophical question about arquillian ;-)? If it is philosophical, I am out - no detailed knowledge about the concepts behind arquillian...

@starksm64
Copy link
Member

The comment was on both as it is not clear what the purpose of tracking by thread is. I can't see this causes any harm and getting rid of questionable ThreadLocal use is a good thing, so I'll merge it.

@starksm64 starksm64 marked this pull request as ready for review July 23, 2024 04:29
@starksm64 starksm64 merged commit 4d182c5 into arquillian:master Jul 23, 2024
16 checks passed
@WolfgangHG WolfgangHG deleted the customthreadlocal branch July 26, 2024 07:56
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.

2 participants