Avoid ThreadLocal memory leak in JoinPointImpl #291
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kriegaex
force-pushed
the
gh-288
branch
3 times, most recently
from
eb4d567 to
c3c2907
Compare
- Recognise more "fork-worthy" JVM parameters - Pass on program arguments to program in forked JVM Signed-off-by: Alexander Kriegisch <[email protected]>
Leak was introduced in commit 3c80a36, fixing #128, but introducing #288 instead, which was the lesser of two evils, but still bad for some users unwilling to use native AspectJ syntax for their aspects, avoiding the problem. Relates to #288. Signed-off-by: Alexander Kriegisch <[email protected]>
according to https://rules.sonarsource.com/java/tag/leak/RSPEC-5164/. Now, there no longer is a thread-local stack of AroundClosure instances, but rather a list of them, which can only grow but never shrink. Instead, we now have a thread-local (integer) list index, for every thread being initialised with pointing to the last element. I.e., every thread can unwind by decrementing the index while proceeding, independently of other threads. A positive side effect is that this approach also works for long-lived threads from thread pools, used by executor services. Hence, test Bugs199Tests.testAsyncProceedNestedAroundAdviceThreadPool_gh128, which was previously commented out, has been activated and passes, see #141. I am not sure if this brings @AspectJ style, non-inlined, nested around advice execution functionally on par with native ones, but at least for current scenarios it seems to work. Fixes #288, #141. Signed-off-by: Alexander Kriegisch <[email protected]>
|
@aclement, knowing your time constraints, I would merge without your review, too, but I want to invite you to take a look, because you also participated in the discussion about #128, in which I introduced the imperfect inheritable thread-local solution which fixed #128, but left #141 open and, as we now know, caused the memory leak in #288. This PR fixes both problems in one change and also adds regression tests, i.e. it is a fix and a feature enhancement at the same time. Because |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
according to RSPEC-5164.
Fixes #288, #141.