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

8309761: Leak class loader constraints #14407

Closed
wants to merge 3 commits into from

Conversation

zhengyu123
Copy link
Contributor

@zhengyu123 zhengyu123 commented Jun 10, 2023

Please review this patch to avoid leaking class loader constraints.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8309761: Leak class loader constraints (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14407/head:pull/14407
$ git checkout pull/14407

Update a local copy of the PR:
$ git checkout pull/14407
$ git pull https://git.openjdk.org/jdk.git pull/14407/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14407

View PR using the GUI difftool:
$ git pr show -t 14407

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14407.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 10, 2023

👋 Welcome back zgu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jun 10, 2023

@zhengyu123 The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jun 10, 2023
@zhengyu123 zhengyu123 marked this pull request as ready for review June 10, 2023 03:03
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 10, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 10, 2023

Webrevs

@jdksjolen
Copy link
Contributor

jdksjolen commented Jun 10, 2023

Hi,

Yes, this fixes the bug, but I'm confused.

If ConstraintSet determines the lifetime of the LoaderConstraints, then why isn't the type of _constraints GrowableArray<LoaderConstraint>*? loaderConstraints.cpp:161 is the only place where we create LoaderConstraints on the heap and also the only place where we call add_constraint(). I'd rather see that change, than adding code onto the destructor. Is there some reason that this can't be done?

Cheers,
Johan

@zhengyu123
Copy link
Contributor Author

Hi,

Yes, this fixes the bug, but I'm confused.

If ConstraintSet determines the lifetime of the LoaderConstraints, then why isn't the type of _constraints GrowableArray<LoaderConstraint>*? loaderConstraints.cpp:161 is the only place where we create LoaderConstraints on the heap and also the only place where we call add_constraint(). I'd rather see that change, than adding code onto the destructor. Is there some reason that this can't be done?

Cheers, Johan

I have not followed this part of code for a while, @coleenp probably is better person to ask.
One potential issue I see by changing GrowableArray<LoaderConstraint*>* to GrowableArray<LoaderConstraint>*, is that, if you have a dangling LoaderConstraint* pointer somewhere, then you call remove_constraint()', that potential overwrites the content of the pointer points to.

Copy link
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

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

Looks good - thank you for fixing this.

@openjdk
Copy link

openjdk bot commented Jun 10, 2023

@zhengyu123 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8309761: Leak class loader constraints

Reviewed-by: coleenp, jsjolen

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

  • 16c3d53: 8308603: Removing do_pending_ref/enclosing_ref from MetaspaceClosure
  • b94b679: 8309627: Incorrect sorting of DirtyCardQueue buffers

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 10, 2023
@iklam
Copy link
Member

iklam commented Jun 11, 2023

Hi,

Yes, this fixes the bug, but I'm confused.

If ConstraintSet determines the lifetime of the LoaderConstraints, then why isn't the type of _constraints GrowableArray<LoaderConstraint>*? loaderConstraints.cpp:161 is the only place where we create LoaderConstraints on the heap and also the only place where we call add_constraint(). I'd rather see that change, than adding code onto the destructor. Is there some reason that this can't be done?

Cheers, Johan

If we make it GrowableArray<LoaderConstraint>*, the management of LoaderConstraint::_loaders would be messy. E.g., if you do

{
   LoaderConstraint lc = _constraints->at(i);
   // lc.~LoaderConstraint() is called here
}

How do we know if lc._loaders should be freed or not?

Copy link
Contributor

@jdksjolen jdksjolen left a comment

Choose a reason for hiding this comment

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

Hi,
Yes, this fixes the bug, but I'm confused.
If ConstraintSet determines the lifetime of the LoaderConstraints, then why isn't the type of _constraints GrowableArray<LoaderConstraint>*? loaderConstraints.cpp:161 is the only place where we create LoaderConstraints on the heap and also the only place where we call add_constraint(). I'd rather see that change, than adding code onto the destructor. Is there some reason that this can't be done?
Cheers, Johan

If we make it GrowableArray<LoaderConstraint>*, the management of LoaderConstraint::_loaders would be messy. E.g., if you do

{
   LoaderConstraint lc = _constraints->at(i);
   // lc.~LoaderConstraint() is called here
}

How do we know if lc._loaders should be freed or not?

E& at(int i) returns a reference, so your code has an implicit pointer dereference. Change the code to the following and we're fine:

{
  LoaderConstraint& lc = _constraints->at(i);
}

This is probably the pattern that we should utilize.

Hi,
Yes, this fixes the bug, but I'm confused.
If ConstraintSet determines the lifetime of the LoaderConstraints, then why isn't the type of _constraints GrowableArray<LoaderConstraint>*? loaderConstraints.cpp:161 is the only place where we create LoaderConstraints on the heap and also the only place where we call add_constraint(). I'd rather see that change, than adding code onto the destructor. Is there some reason that this can't be done?
Cheers, Johan

I have not followed this part of code for a while, @coleenp probably is better person to ask. One potential issue I see by changing GrowableArray<LoaderConstraint*>* to GrowableArray<LoaderConstraint>*, is that, if you have a dangling LoaderConstraint* pointer somewhere, then you call remove_constraint()', that potential overwrites the content of the pointer points to.

Thanks for the reply. That's true, but I don't see how we know whether we have a dangling pointer by the end of your destructor or not either. References aren't a 100% guarantee for that either, but it is an improvement.

I don't want to delay this PR further. It does solve an issue and Coleen approves of it. I do think that we should reconsider the choices we've made in a future RFE, however.

Cheers,
Johan

@zhengyu123
Copy link
Contributor Author

zhengyu123 commented Jun 11, 2023

Thanks for the reply. That's true, but I don't see how we know whether we have a dangling pointer by the end of your destructor or not either. References aren't a 100% guarantee for that either, but it is an improvement.

Reference or pointer make no difference and it has nothing to do with destructor. Consider following scenario:

There are 3 constraints in _constraints array, c0, c1 and c2 and there is an outstanding reference r = &c1. If there is a call to remove_contraint(c0), _constraints array shifts to left, now r refers to c2, instead of c1.

@jdksjolen
Copy link
Contributor

Thanks for the reply. That's true, but I don't see how we know whether we have a dangling pointer by the end of your destructor or not either. References aren't a 100% guarantee for that either, but it is an improvement.

Reference or pointer make no difference and it has nothing to do with destructor. Consider following scenario:

There are 3 constraints in _constraints array, c0, c1 and c2 and there is an outstanding reference r = &c1. If there is a call to remove_contraint(c0), _constraints array shifts to left, now r refers to c2, instead of c1.

Alright, that makes a lot of sense. Thank you for explaining :).

@zhengyu123
Copy link
Contributor Author

Thanks for the reviews, @coleenp @jdksjolen

@zhengyu123
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 12, 2023

Going to push as commit 8e4e6b0.
Since your change was applied there have been 12 commits pushed to the master branch:

  • fdaa2c4: 8309306: G1: Move is_obj_dead from HeapRegion to G1CollectedHeap
  • 4bc6bbb: 8309814: [IR Framework] Dump socket output string in which IR encoding was not found
  • cf9e635: 8309462: [AIX] vmTestbase/nsk/jvmti/RunAgentThread/agentthr001/TestDescription.java crashing due to empty while loop
  • 268ec61: 8308184: Launching java with large number of jars in classpath with java.protocol.handler.pkgs system property set can lead to StackOverflowError
  • 4d47069: 4516654: Metalworks Demo: Window title not displayed fully in Low Vision Theme
  • 408cadb: 8309467: Pattern dominance should be adjusted
  • 6c3e621: 8308749: C2 failed: regular loops only (counted loop inside infinite loop)
  • f5cbe53: 8027711: Unify wildcarding syntax for CompileCommand and CompileOnly
  • 4d66d97: 8309549: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java fails on AIX
  • 3981297: 8309703: AIX build fails after JDK-8280982
  • ... and 2 more: https://git.openjdk.org/jdk/compare/aace3dc28c577bae67a6a1d376a514740d752928...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 12, 2023
@openjdk openjdk bot closed this Jun 12, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 12, 2023
@openjdk
Copy link

openjdk bot commented Jun 12, 2023

@zhengyu123 Pushed as commit 8e4e6b0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@coleenp
Copy link
Contributor

coleenp commented Jun 12, 2023

Just reading this now. The reason it's

GrowableArray<LoaderConstraint*>*  _constraints;   // loader constraints for this class name.

and not

  GrowableArray<LoaderConstraint>*  _constraints;   // loader constraints for this class name.

Was basically that it's simpler to understand and manipulate the constraint list as pointers. Except sadly had a memory leak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
4 participants