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.

Sign up for GitHub

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

Jump to bottom

FISH-7204 Removes internal ASURLClassLoader throwable reference to avoid ClassLoader leaks #6206

Merged
merged 1 commit into from
May 10, 2023
Merged

FISH-7204 Removes internal ASURLClassLoader throwable reference to avoid ClassLoader leaks #6206

merged 1 commit into from
May 10, 2023

Conversation

ctabin
Copy link
Contributor

@ctabin ctabin commented Feb 16, 2023

Description

Fixes potential ClassLoader leaks that might be kept in the StackTrace reference of the internal Throwable in ASURLClassLoader.

Here is what we have in visualvm:

screen

One can also note there is a lot of cleaner for SentinelInputStream pending.

Probably due to #6179.

Environment

  • payara-embedded-all 6.2023.2
  • JDK17
  • Linux Debian Bullseye

@ctabin ctabin changed the title Removes internal throable reference to avoid potential ClassLoader leaks Removes internal throwable reference to avoid potential ClassLoader leaks Feb 16, 2023
@ctabin
Copy link
Contributor Author

ctabin commented Feb 17, 2023

Jenkins test please

@ctabin
Copy link
Contributor Author

ctabin commented Feb 17, 2023

Error seems to be unrelated to this PR: Could not resolve dependencies for project fish.payara.samples:ejb-http-remoting:war:6.2023.3-SNAPSHOT: Failed to collect dependencies at org.glassfish.mq:imq:jar:6.1.1: Failed to read artifact descriptor for org.glassfish.mq:imq:jar:6.1.1: Could not transfer artifact org.glassfish.mq:imq:pom:6.1.1 from/to zulu-jdk (https://nexus.payara.fish/content/repositories/zulu-jdk/): Authentication failed for https://nexus.payara.fish/content/repositories/zulu-jdk/org/glassfish/mq/imq/6.1.1/imq-6.1.1.pom 401 Unauthorized

@aubi
Copy link
Contributor

aubi commented Feb 17, 2023

We are fixing it and we'll restart the build.

@aubi
Copy link
Contributor

aubi commented Feb 21, 2023

Build is fixed

@ctabin
Copy link
Contributor Author

ctabin commented Feb 27, 2023

Thanks @aubi. Who may I ask for a review ?

@ctabin
Copy link
Contributor Author

ctabin commented Mar 7, 2023

@aubi @lprimak Is there a chance to get a review in order to get it merged for the next release ? Thanks.

@lprimak
Copy link
Contributor

lprimak commented Mar 7, 2023

It's up to Payara folks. I can review it :)

@ctabin
Copy link
Contributor Author

ctabin commented Mar 20, 2023

@breakponchito Could you take a look or assign to someone ? Thanks and sorry for the bothering.

@ctabin
Copy link
Contributor Author

ctabin commented Mar 30, 2023

For information, this problem is still present in 6.2023.3.

@ctabin ctabin changed the title Removes internal throwable reference to avoid potential ClassLoader leaks Removes internal ASURLClassLoader throwable reference to avoid ClassLoader leaks Mar 30, 2023
@ctabin
Copy link
Contributor Author

ctabin commented Mar 31, 2023

Jenkins test please

@ctabin ctabin mentioned this pull request Apr 2, 2023
Merged
@ctabin
Copy link
Contributor Author

ctabin commented Apr 7, 2023

@shub8968 Since those modifications have been merged in GlassFish, can you assign someone ? It should be pretty straightforward and prevents us to upgrade from 6.2023.1. 🙏

@ctabin
Copy link
Contributor Author

ctabin commented Apr 17, 2023

@JamesHillyard Would it be possible to also raise an internal ticket for this please ? 🙏

@JamesHillyard JamesHillyard changed the title Removes internal ASURLClassLoader throwable reference to avoid ClassLoader leaks FISH-7332 Removes internal ASURLClassLoader throwable reference to avoid ClassLoader leaks Apr 17, 2023
@JamesHillyard
Copy link
Member

JamesHillyard commented Apr 17, 2023 •
edited
Loading

Hi @ctabin,

Yes of course - The internal issue FISH-7332 has been raised to track this issue. Turns out this already had an internal issue, it just wasn't updated on GitHub - This is being tracked by FISH-7204 Thank you again for your contribution and your patience while we find time to test and review your contributions

@JamesHillyard JamesHillyard changed the title FISH-7332 Removes internal ASURLClassLoader throwable reference to avoid ClassLoader leaks FISH-7204 Removes internal ASURLClassLoader throwable reference to avoid ClassLoader leaks Apr 17, 2023
@JamesHillyard JamesHillyard added the PR: Awaiting CLA Contributor does not have a CLA or has submitted an unconfirmed CLA. label Apr 21, 2023
@smillidge smillidge added PR: CLA CLA submitted on PR by the contributor and removed PR: Awaiting CLA Contributor does not have a CLA or has submitted an unconfirmed CLA. labels Apr 21, 2023
@smillidge
Copy link
Contributor

CLA received

aubi
aubi approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@aubi aubi left a comment

Choose a reason for hiding this comment

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

Approved, sorry for the long time.

@aubi aubi merged commit 23d1865 into payara:master May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aubi aubi aubi approved these changes

@lprimak lprimak lprimak approved these changes

Assignees
No one assigned
Labels
PR: CLA CLA submitted on PR by the contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ctabin @aubi @lprimak @JamesHillyard @smillidge