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

FISH-8672 bugfix: removed class loader leaks #6677

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

lprimak
Copy link
Contributor

@lprimak lprimak commented May 2, 2024

Description

Fixes ClassLoader leaks
fixes #6554 (partial fix, performance isn't addressed, just leaks)
fixes #6301
fixes #4098

Important Info

Testing

Tested with appserver/tests/payara-samples/classloader-data-api leak tester app and VisualVM

Testing Environment

Any

@lprimak lprimak marked this pull request as draft May 2, 2024 20:18
@lprimak lprimak marked this pull request as ready for review May 2, 2024 23:00
@RInverid
Copy link

RInverid commented May 7, 2024

Testing locally, this MR does not address the memory issues mentioned in #6554

@lprimak
Copy link
Contributor Author

lprimak commented May 7, 2024

@RInverid Can you please elaborate? Do you have a reproducer that you are using?
Do you have any "GC Root" output from VisualVM?

My applications that I am testing with may not hit all of the leaks you are experiencing, as it doesn't use all of Jakarta EE APIs and only simple configurations.

Thank you for your help.

@lprimak
Copy link
Contributor Author

lprimak commented May 7, 2024

You can also use this pre-built binary: https://nexus.hope.nyc.ny.us/repository/maven-releases/fish/payara/distributions/payara/6.2024.5-9/payara-6.2024.5-9.zip
Docker image is also available: docker pull lprimak/payara-full

@lprimak lprimak marked this pull request as draft May 7, 2024 17:50
@lprimak lprimak force-pushed the fix-leaks branch 8 times, most recently from 5d93259 to 4f1ed62 Compare May 8, 2024 06:12
@lprimak lprimak marked this pull request as ready for review May 8, 2024 06:13
@lprimak
Copy link
Contributor Author

lprimak commented May 8, 2024

Found more leaks in Jax-RS EJB, and EAR deployment code. Should be fixed now.

@RInverid
Copy link

RInverid commented May 8, 2024

@RInverid Can you please elaborate? Do you have a reproducer that you are using? Do you have any "GC Root" output from VisualVM?

My applications that I am testing with may not hit all of the leaks you are experiencing, as it doesn't use all of Jakarta EE APIs and only simple configurations.

Thank you for your help.

I have our own application that I'm testing deployment time and memory usage on, which is how I found the commit that caused #6554 . I'll re-run with your updated branch and let you know the results!

@RInverid
Copy link

RInverid commented May 8, 2024

Update: the issue of too much memory usage & long deployment time is still present for me

@lprimak
Copy link
Contributor Author

lprimak commented May 8, 2024

Update: the issue of too much memory usage & long deployment time is still present for me

The performance regression is not in scope for this PR.

All my applications run without memory leaks now and can be redeployed 1000s of times without running out of memory.
As I said before, they don't touch all of Payara's features and there may be another leak that isn't detected here.

What I suggest is to either provide a simple reproducer that shows the leak, or a "GC Root" screenshot from ClassLoader so I can be of some help.

There is also a possibility that your application or your setup is leaking memory somehow as well.

@lprimak
Copy link
Contributor Author

lprimak commented May 8, 2024

I did test with reproducer from #6554 and that works without leaks now.

@Pandrex247 Pandrex247 added the PR: CLA CLA submitted on PR by the contributor label May 8, 2024
@Pandrex247 Pandrex247 changed the title bugfix: removed class loader leaks FISH-8672 bugfix: removed class loader leaks May 8, 2024
@lprimak
Copy link
Contributor Author

lprimak commented Jun 5, 2024

Looks like #6637 introduced a regression. There are now dangling threads, and this PR needs to be updated to make class ContainerBackgroundSessionProcessor static as well

@lprimak lprimak marked this pull request as ready for review June 8, 2024 03:03
@lprimak
Copy link
Contributor Author

lprimak commented Jun 8, 2024

Fixed regressions. Ready to go.

@kalinchan kalinchan merged commit 9afc1d4 into payara:master Jun 17, 2024
1 check passed
@lprimak lprimak deleted the fix-leaks branch June 18, 2024 18:34
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jun 19, 2024
FISH-8672 bugfix: removed class loader leaks
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jun 26, 2024
FISH-8672 bugfix: removed class loader leaks
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jul 2, 2024
FISH-8672 bugfix: removed class loader leaks
Pandrex247 pushed a commit to Pandrex247/Payara that referenced this pull request Jul 5, 2024
FISH-8672 bugfix: removed class loader leaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
4 participants