8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature #16363

kevinjwalls · 2023-10-25T17:02:03Z

Remove the MLet feature and its tests.

Some tests use MLet classes but are not testing MLets. These are updated, to use another test MBean or an MBean which is a URLClassLoader, e.g.
test/jdk/javax/management/MBeanServer/PostExceptionTest.java
test/jdk/javax/management/remote/mandatory/loading/TargetMBeanTest.java

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8318710 to be approved
Commit message must refer to an issue

Issues

JDK-8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature (Enhancement - P2)
JDK-8318710: Remove the Java Management Extension (JMX) Management Applet (m-let) feature (CSR)

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer) ⚠️ Review applies to 29ec31f2
Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16363

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-10-25T17:03:19Z

👋 Welcome back kevinw! 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 · 2023-10-25T17:03:52Z

@kevinjwalls The following labels will be automatically applied to this pull request:

jmx
serviceability

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

bridgekeeper · 2023-12-20T22:55:01Z

@kevinjwalls This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

kevinjwalls · 2023-12-21T11:20:20Z

Additional test updates, a few more to come...

mlbridge · 2024-01-02T15:54:08Z

Webrevs

test/jdk/javax/management/Introspector/ClassLeakTest.java

test/jdk/javax/management/remote/mandatory/connection/IdleTimeoutTest.java

src/java.management/share/classes/javax/management/loading/MLet.java

sspitsyn

Kevin, thank you for the answers! Looks okay to me.

openjdk · 2024-01-08T18:51:20Z

@kevinjwalls 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:

8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature

Reviewed-by: sspitsyn, dfuchs

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 167 new commits pushed to the master branch:

be900f1: 8323425: JFR: Auto-generated filename doesn't work with time-limited recording
68c4286: 8323008: filter out harmful -std* flags added by autoconf from CXX
7dc9dd6: 8234502: Merge GenCollectedHeap and SerialHeap
ed18222: 8323190: Segfault during deoptimization of C2-compiled code
3e19bf8: 8323529: Relativize test image dependencies in microbenchmarks
ba23025: 8322957: Generational ZGC: Relocation selection must join the STS
7c3a39f: 8323297: Fix incorrect placement of precompiled.hpp include lines
e72723d: 8323296: java/lang/Thread/virtual/stress/GetStackTraceALotWhenPinned.java#id1 timed out
66520be: 8280056: gtest/LargePageGtests.java#use-large-pages failed "os.release_one_mapping_multi_commits_vm"
82a63a0: 8258979: The image didn't show correctly with GTK LAF
... and 157 more: https://git.openjdk.org/jdk/compare/a5cf4210cd9c293a9e9bce60dc6d0f08fd838c77...master

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.

dfuch · 2024-01-10T13:55:51Z

test/jdk/javax/management/Introspector/ClassLeakTest.java

-        testLoader = null;
+        ObjectName testName = new ObjectName("x:name=Test");
+        Test mbean = new Test();
+        mbs.registerMBean(mbean, testName);


I suspect this test should first register an MBean which is a ClassLoader implementing PrivateClassLoader, through which the TestMBean would be loaded in order to preserve the semantics of the test.

In other words, replace the MLet with a regular MBean that extends URLClassLoader and implements PrivateClassLoader and do the same thing than the original test was doing.

I didn't see great value in it, as we no longer provide an MBean which is a ClassLoader.
Having a test MBean that extends URLClassLoader, and calling its loadClass()... is basically testing URLClassLoader? 8-)
Implementing PrivateClassLoader affects ClassLoaderRepository behaviour, but that's not tested here.
If you think this has value, we can do it.

My thinking is that this tests for a leak when an MBean is created using a ClassLoader which is also an MBean. MLet was such a ready-to-use ClassLoader. We no longer have MLets, but it's still possible to register an MBean that is a class loader and use that to create another MBean whose concrete class gets loaded by that ClassLoader MBean. We're testing that the Introspector doesn't create a leak in this case. And I believe the fact that the ClassLoader MBean is a PrivateClassLoader may also be of importance in the tested scenario (maybe).

Checking on the original problem noted in the test, looks like it wasn't really about ClassLoaders and MLets:
JDK-4909536: Standard MBean introspector keeps reference to last class introspected
This is what the test still tests.
Do we see value in having the MBean be a ClassLoader and checking if such MBeans are held on to...? 8-)

Yes, that's the whole point of the test. Without using an MBean which is a ClassLoader the fix cannot be tested.

you need to create an MBean which is an URLClassLoader, and it needs to be a PrivateClassLoader so that it's not added to the ClassLoaderRepository - let's call it LoaderMBean.

you need to use that LoaderMBean to load another MBean - let's call it TestMBean. The concrete class of TestMBean needs to be loaded by the LoaderMBean - that is - LoaderMBean needs to be the defining ClassLoader for that class (the TestMBean class must not be loaded by the System/Application ClassLoader).

In the MBeanServer you then end up with two MBeans, one is the LoaderMBean, the other is the TestMBean whose class has been loaded by the LoaderMBean.

you then keep a weak reference on the LoaderMBean, and unregister both MBeans.

at that point - if the Introspector still has a strong reference to the class of the TestMBean, then the LoaderMBean WeakReference will never be enqueued, because the class of the TestMBean will have a strong reference to its ClassLoader, which is LoaderMBean.

So you can only test that if you have an MBean which is loaded by a custom ClassLoader, because you want to verify that the Introspector doesn't hold on that ClassLoader (through the TestMBean class).

Hi Daniel -
I am missing see how being a ClassLoader relates to the leak where the Introspector holds on to a reference to the most recent MBean. I didn't locate the jdk5 change, I only read a short synopsis.

I can make this test create an MBean which is a ClassLoader, and the test can do its check that having the loader MBean load a class, creates a distinct class. I can add that update here. But it looks like an MLet test which leaked into the Introspector test. 8-)

Hi Kevin,

To verify whether the Introspector holds a reference to a Class Object you need that class to be loaded by a custom class loader, and verify that the custom class loader that loaded that class can be garbage collected.

If the class loader that loaded that class has not been garbage collected after you released all references to it, then it's because the class it loaded is still referenced somewhere.

The logic of the test is as follows:
You create an MBean L which is a class loader and able to load class C. L must be the defining class loader for C.
You register L in the MBeanServer.
You ask the MBeanServer to create an MBean of class C through L (this is why you must use the variant of createMBean that takes a loader name).
You keep a weak ref to L
You unregister L and the MBean of class C from the MBeanServer and make sure the test doesn't have any strong reference to them (or to class C).
Then you start GC and wait until the weak ref to L is enqueued. If that doesn't happen, it's because the Introspector still hold a reference to class C.

You cannot test that if C has been loaded by the system class loader.

I am updating those two tests.
I read the test as checking that an Object instance is collected, rather than Class Object, so was favouring making it simpler.
But we can use an MBean which is a URLClassLoader, and things will be more similar to the originals.

Some of MXBeanLoadingTest1 is duplicating some of ClassLeakTest, so something for future auditing perhaps...

dfuch · 2024-01-10T14:02:25Z

test/jdk/javax/management/mxbean/MXBeanLoadingTest1.java

-            }
-            testLoader = null;
-
+            ObjectInstance mb = mbs.createMBean(Test.class.getName(), testName);


Same remark here - I think we need to first register an MBean which is a ClassLoader and implements PrivateClassLoader to replace the MLet in this test.

dfuch

That looks better. I would have kept the LoaderMBean and the TestMBean separate - as using the same class for both makes the test harder to understand (it's not obvious that there are two separate copies of the TestMBean class, one loaded by the System ClassLoader, one loaded by the TestMBean itself when acting as a loader), but otherwise looks good.

kevinjwalls · 2024-01-15T11:09:49Z

Thanks Daniel and Serguei for reviewing!

kevinjwalls · 2024-01-15T11:11:10Z

/integrate

openjdk · 2024-01-15T11:12:39Z

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

cd0fe37: 8323641: Test compiler/loopopts/superword/TestAlignVectorFuzzer.java timed out
45c65e6: 8323577: C2 SuperWord: remove AlignVector restrictions on IR tests added in JDK-8305055
8643cc2: 8323610: G1: HeapRegion pin count should be size_t to avoid overflows
e66a76f: 8323660: Serial: Fix header ordering and indentation
ba3c3bb: 8323519: Add applications/ctw/modules to Hotspot tiered testing
922f8e4: 8323693: Update some copyright announcements in the new files created in 8234502
1515bd7: 8322077: Add Ideal transformation: (~a) | (~b) => ~(a & b)
bdee968: 4760025: sRGB conversions to and from CIE XYZ incorrect
71d9a83: 8323243: JNI invocation of an abstract instance method corrupts the stack
d83ea92: 8301466: [AIX] Revisit CommittedVirtualMemoryTest
... and 179 more: https://git.openjdk.org/jdk/compare/a5cf4210cd9c293a9e9bce60dc6d0f08fd838c77...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-01-15T11:12:48Z

@kevinjwalls Pushed as commit 8c238ed.

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

kevinjwalls added 7 commits October 25, 2023 14:55

Basic delete

9d0cfc8

MBSAC

2c3c9c0

MBSAC

f1ba54c

ClassLoaderRepositorySupport comment

ad85157

MLet test removal

0be0a62

remove LibraryLoader MLet test

cfc673b

Test update

f136375

openjdk bot added the csr Pull request needs approved CSR before integration label Oct 25, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org labels Oct 25, 2023

openjdk bot removed the csr Pull request needs approved CSR before integration label Dec 7, 2023

Merge remote-tracking branch 'upstream/master' into 8318707_MLet_Remove

8b6762e

kevinjwalls added 2 commits December 21, 2023 10:18

Remove PrivateMLet from ClassleakTest, retain stated purpose of test

7646e4f

Addtional test updates

51d81af

kevinjwalls added 5 commits December 21, 2023 12:43

Further test updates

2636b16

Remove MLet from additional tests

56b006d

(C) updates

34035ef

Merge remote-tracking branch 'upstream/master' into 8318707_MLet_Remove

167ba8c

remote old commented-out code

cc6e0d2

kevinjwalls marked this pull request as ready for review January 2, 2024 15:49

openjdk bot added the rfr Pull request is ready for review label Jan 2, 2024

sspitsyn reviewed Jan 4, 2024

View reviewed changes

test/jdk/javax/management/Introspector/ClassLeakTest.java Outdated Show resolved Hide resolved

sspitsyn reviewed Jan 4, 2024

View reviewed changes

test/jdk/javax/management/remote/mandatory/connection/IdleTimeoutTest.java Outdated Show resolved Hide resolved

ecki reviewed Jan 4, 2024

View reviewed changes

src/java.management/share/classes/javax/management/loading/MLet.java Show resolved Hide resolved

IdleTimeoutTest ObjectName more like original

ec9f539

Remove unnecessary MLET_ Properties

29ec31f

sspitsyn approved these changes Jan 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 8, 2024

dfuch reviewed Jan 10, 2024

View reviewed changes

Test update using URLClassLoader MBeans, more like original tests

70092fa

dfuch approved these changes Jan 12, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 15, 2024

openjdk bot closed this Jan 15, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature #16363

8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature #16363

kevinjwalls commented Oct 25, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 25, 2023

openjdk bot commented Oct 25, 2023

bridgekeeper bot commented Dec 20, 2023

kevinjwalls commented Dec 21, 2023

mlbridge bot commented Jan 2, 2024 •

edited

Loading

sspitsyn left a comment

openjdk bot commented Jan 8, 2024 •

edited

Loading

dfuch Jan 10, 2024

kevinjwalls Jan 10, 2024 •

edited

Loading

dfuch Jan 10, 2024 •

edited

Loading

kevinjwalls Jan 10, 2024

dfuch Jan 10, 2024 •

edited

Loading

kevinjwalls Jan 11, 2024 •

edited

Loading

dfuch Jan 11, 2024 •

edited

Loading

kevinjwalls Jan 11, 2024

dfuch Jan 10, 2024

kevinjwalls Jan 11, 2024

dfuch left a comment

kevinjwalls commented Jan 15, 2024

kevinjwalls commented Jan 15, 2024

openjdk bot commented Jan 15, 2024

openjdk bot commented Jan 15, 2024

8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature #16363

8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature #16363

Conversation

kevinjwalls commented Oct 25, 2023 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 25, 2023

openjdk bot commented Oct 25, 2023

bridgekeeper bot commented Dec 20, 2023

kevinjwalls commented Dec 21, 2023

mlbridge bot commented Jan 2, 2024 • edited Loading

Webrevs

sspitsyn left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 8, 2024 • edited Loading

dfuch Jan 10, 2024

Choose a reason for hiding this comment

kevinjwalls Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

dfuch Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

kevinjwalls Jan 10, 2024

Choose a reason for hiding this comment

dfuch Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

kevinjwalls Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

dfuch Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

kevinjwalls Jan 11, 2024

Choose a reason for hiding this comment

dfuch Jan 10, 2024

Choose a reason for hiding this comment

kevinjwalls Jan 11, 2024

Choose a reason for hiding this comment

dfuch left a comment

Choose a reason for hiding this comment

kevinjwalls commented Jan 15, 2024

kevinjwalls commented Jan 15, 2024

openjdk bot commented Jan 15, 2024

openjdk bot commented Jan 15, 2024

kevinjwalls commented Oct 25, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 2, 2024 •

edited

Loading

openjdk bot commented Jan 8, 2024 •

edited

Loading

kevinjwalls Jan 10, 2024 •

edited

Loading

dfuch Jan 10, 2024 •

edited

Loading

dfuch Jan 10, 2024 •

edited

Loading

kevinjwalls Jan 11, 2024 •

edited

Loading

dfuch Jan 11, 2024 •

edited

Loading