Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX [v4]
On Tue, 10 Sep 2024 13:47:47 GMT, Kevin Walls wrote: >> Remove very very old serialization compatibility logic from JMX. >> >> This relates to keeping the JDK's JMX implementation compatible with JMX >> versions from when JMX was a separate component, before being integrated >> into JDK 5. It should all be removed. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Replace the missing serialField comments Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20856#pullrequestreview-2297493614
Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX [v4]
On Tue, 10 Sep 2024 13:47:47 GMT, Kevin Walls wrote: >> Remove very very old serialization compatibility logic from JMX. >> >> This relates to keeping the JDK's JMX implementation compatible with JMX >> versions from when JMX was a separate component, before being integrated >> into JDK 5. It should all be removed. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Replace the missing serialField comments OK - thanks. LGTM now. - PR Review: https://git.openjdk.org/jdk/pull/20856#pullrequestreview-2292612344
Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX [v3]
On Tue, 10 Sep 2024 13:06:25 GMT, Kevin Walls wrote: >> Remove very very old serialization compatibility logic from JMX. >> >> This relates to keeping the JDK's JMX implementation compatible with JMX >> versions from when JMX was a separate component, before being integrated >> into JDK 5. It should all be removed. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Remove a list level in Javadoc. Changes requested by dfuchs (Reviewer). src/java.management/share/classes/javax/management/modelmbean/ModelMBeanNotificationInfo.java line 104: > 102: > 103: private static final long serialVersionUID = -7445681389570207141L; > 104: private static final ObjectStreamField[] serialPersistentFields = Same here: Suggestion: /** * @serialField notificationDescriptor Descriptor The descriptor * containing the appropriate metadata for this instance */ private static final ObjectStreamField[] serialPersistentFields = - PR Review: https://git.openjdk.org/jdk/pull/20856#pullrequestreview-2292546453 PR Review Comment: https://git.openjdk.org/jdk/pull/20856#discussion_r1751983528
Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX [v3]
On Tue, 10 Sep 2024 13:06:25 GMT, Kevin Walls wrote:
>> Remove very very old serialization compatibility logic from JMX.
>>
>> This relates to keeping the JDK's JMX implementation compatible with JMX
>> versions from when JMX was a separate component, before being integrated
>> into JDK 5. It should all be removed.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove a list level in Javadoc.
Changes requested by dfuchs (Reviewer).
src/java.management/share/classes/javax/management/modelmbean/ModelMBeanAttributeInfo.java
line 124:
> 122:
> 123: private static final long serialVersionUID =
> 6181543027787327345L;
> 124: private static final ObjectStreamField[] serialPersistentFields =
API doc coment for serial persistent fields should have been preserved.
Suggestion:
/**
* @serialField attrDescriptor Descriptor The {@link Descriptor}
* containing the metadata corresponding to this attribute
*/
private static final ObjectStreamField[] serialPersistentFields =
-
PR Review: https://git.openjdk.org/jdk/pull/20856#pullrequestreview-2292536179
PR Review Comment: https://git.openjdk.org/jdk/pull/20856#discussion_r1751974983
Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX [v3]
On Tue, 10 Sep 2024 13:06:25 GMT, Kevin Walls wrote:
>> Remove very very old serialization compatibility logic from JMX.
>>
>> This relates to keeping the JDK's JMX implementation compatible with JMX
>> versions from when JMX was a separate component, before being integrated
>> into JDK 5. It should all be removed.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove a list level in Javadoc.
src/java.management/share/classes/javax/management/modelmbean/InvalidTargetObjectTypeException.java
line 52:
> 50: {
> 51: private static final long serialVersionUID = 1190536278266811217L;
> 52: private static final ObjectStreamField[] serialPersistentFields =
The API doc comment for the serialized fields shouldhave been preserved.
Suggestion:
/**
* @serialField exception Exception Encapsulated {@link Exception}
*/
private static final ObjectStreamField[] serialPersistentFields =
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20856#discussion_r1751967011
Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX [v3]
On Tue, 10 Sep 2024 13:06:25 GMT, Kevin Walls wrote: >> Remove very very old serialization compatibility logic from JMX. >> >> This relates to keeping the JDK's JMX implementation compatible with JMX >> versions from when JMX was a separate component, before being integrated >> into JDK 5. It should all be removed. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Remove a list level in Javadoc. Thanks, the serial form looks much nicer now. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20856#pullrequestreview-2292457896
Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX [v2]
On Thu, 5 Sep 2024 17:03:37 GMT, Kevin Walls wrote: >> Remove very very old serialization compatibility logic from JMX. >> >> This relates to keeping the JDK's JMX implementation compatible with JMX >> versions from when JMX was a separate component, before being integrated >> into JDK 5. It should all be removed. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test LGTM - might be good to have an other pait of eyes looking at it before integrating... - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20856#pullrequestreview-2286180154
Re: RFR: 8334165: Remove serialVersionUID compatibility logic from JMX
On Wed, 4 Sep 2024 16:27:51 GMT, Kevin Walls wrote: > Remove very very old serialization compatibility logic from JMX. > > This relates to keeping the JDK's JMX implementation compatible with JMX > versions from when JMX was a separate component, before being integrated into > JDK 5. It should all be removed. Wow. I hadn't realized so many classes were impacted. I thought there was just a few! Look good in general. It could be good to write a test that sets the property with 1.0 and 1.1, and that verifies that something (e.g. an ObjectName?) serialized with property=1.0 can be deserialized with property=1.1, and conversely. That would validate that setting the property no longer has any effect. - PR Review: https://git.openjdk.org/jdk/pull/20856#pullrequestreview-2283335277
Re: RFR: 8338817: Wrong indent in API docs for java.lang.management.ManagementFactory
On Mon, 2 Sep 2024 11:50:11 GMT, Kevin Walls wrote: > Trivial change. > Javadoc has a misplaced and unnecessary blockquote tag which cuts across two > sections. Removing this fixes the odd indent. Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20813#pullrequestreview-2275946996
Re: RFR: 8335625: Update Javadoc for GetCpuLoad [v2]
On Wed, 21 Aug 2024 13:42:18 GMT, Joakim Nordström
wrote:
>> Can I get a review of this documentation update to clarify the usage of
>> GetCpuLoad (and inherently deprecated GetSystemCpuLoad) and
>> GetProcessCpuLoad.
>>
>> Calling either of these methods in quick succession can lead to
>> unrepresentative results due to too few data points.
>>
>> This behavior is easy to reproduce on at least Linux (Windows implementation
>> enforces a 500 ticks duration); when calling GetCpuLoad repeatedly CPU load
>> values of either 0, 0.5, or 1 will be returned.
>>
>> double cpuLoad1 = getCpuLoad();
>> double cpuLoad2 = getCpuLoad(); // not enough ticks has passed to give
>> representative values
>>
>> Worth noting is that this holds true even if getSystemCpuLoad() is called.
>>
>> double cpuLoad1 = getCpuLoad();
>> double cpuLoad2 = getSystemCpuLoad(); // not enough ticks has passed to
>> give representative values, since getSystemCpuLoad effectively calls
>> getCpuLoad.
>
> Joakim Nordström has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Made it an apiNote
src/jdk.management/share/classes/com/sun/management/OperatingSystemMXBean.java
line 145:
> 143: * call made to this method, or {@link #getCpuLoad()}. For the very
> first
> 144: * invocation of this method, the recent period of observation is
> undefined.
> 145: *
I wonder if the text would read better as:
* @apiNote The recent period of observation is typically the duration
since the last
Since this is an MBean, many different callers could be calling this method,
including other Monitor MBeans that may have been registered. Adding
_typically_ will soften the assertion, give more leeway to implementations, and
still be informative enough for readers to better understand the API?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20546#discussion_r1725176645
Re: [jdk23] RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed
On Thu, 20 Jun 2024 15:24:35 GMT, Kevin Walls wrote: > 844: JMX attaching of Subject does not work when security manager not > allowed LGTM - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19810#pullrequestreview-2132226211
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v19]
On Tue, 18 Jun 2024 12:17:46 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Additional test runs with SM enabled The code changes look good to me (if a bit verbose) and the test changes look reasonable. It could be beneficial to add some more tests in the future involving monitoring and getting the subject from within a monitored MBean. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2127943873
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 20:27:05 GMT, Sean Mullan wrote: >> Hi, >> We almost always check >> !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the >> RMIConnectionImpl contstructor) >> >> This keeps the "modern" case first (except in that constructor, where it is >> only one line for each side of the if...). >> >> This extra checking of >> SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the >> modern and old style cases distinct, based on other comments here. It keeps >> it simple to read I hope, but yes it is a little more verbose than it could >> be. >> >> I'm hoping you'll agree that as these checks will be removed anyway, >> probably with a release, we should delay more extensive cleanup until then. >> (I've also rolled back my noPermissionsACC removal to keep this change away >> from being a general cleanup.) >> >> I had a bunch of additional Exception handling for e.g. >> PrivilegedActionException I think in here, and it wasn't very pretty. But, >> it might look better when there are fewer nested ifs in these methods, and >> when we are properly in the post-SecurityManager world... 8-) >> >> Whether it checks acc != null or acc == null is based on the existing code. >> I think that makes this diff easier to read, but also could be reworked and >> be made more consistent after SM removal. > > Agree with Kevin. To minimize risk, especially since this is to fix a > significant regression and we are in RDP1, we are really trying to preserve > the existing code as much as possible, even though it could be improved. It is also more error prone (which is why I suggested using a single well tested utility method to implement the temporary hackery rather than spreading it at different places in different forms). But I'm OK with the code in this form. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644303322
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 12:54:44 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with two additional > commits since the last revision: > > - style update > - whitespace The new version looks much better to me. I wonder if we can abstract away some of the repeated logic though. src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java line 1314: > 1312: return AccessController.doPrivileged(action, acc); > 1313: } > 1314: } This piece of code is repeated at several places in similar but different forms - sometime we check for `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check for `acc == null`, sometime for `acc != null` etc.. I wonder if we could abstract that to some utility method somewhere. Would that make sense? Maybe that's not possible due to using sometime `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we could use `PrivilegedExceptionAction` everywhere at the cost of handling `PrivilegedActionException`? If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds again an equivalent amount of clutter) then maybe we could at least make sure we do the same checks in the same way (typically `acc == null` vs `acc != null`)? - PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2118725434 PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642864523
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v13]
On Fri, 14 Jun 2024 15:26:54 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Unnecessary catches to remove
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1437:
> 1435: } catch (Exception e) {
> 1436: if (e instanceof RuntimeException)
> 1437:throw (RuntimeException) e;
Suggestion:
if (e instanceof RuntimeException rte)
throw rte;
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1450:
> 1448: } catch (Exception e) {
> 1449: if (e instanceof RuntimeException)
> 1450:throw (RuntimeException) e;
Suggestion:
if (e instanceof RuntimeException rte)
throw rte;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1640033632
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1640034429
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v6]
On Wed, 12 Jun 2024 16:11:28 GMT, Kevin Walls wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
>Undo test policy updates
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1445:
> 1443: } else {
> 1444: throw new PrivilegedActionException(e);
> 1445: }
I assume there no chance that `Exception e` may already be a
`PrivilegedActionException` here. You coud avoid casts by using instanceof
patterns.
Suggestion:
if (e instanceof RuntimeException rte) {
throw rte;
} else {
throw new PrivilegedActionException(e);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636791497
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
On Wed, 12 Jun 2024 14:44:56 GMT, Kevin Walls wrote: >> I think Daniel is right, can you remove this permission and paste in the >> debug output to see where this is happening? > > Hmm I may have fixed that since changing the policy files, as I'm not seeing > the problem without that AuthPermission any more. Am just retesting > everything before updating this... (Same with other policy files in which the permission was added of course) - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636634416
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v4]
On Wed, 12 Jun 2024 14:01:40 GMT, Kevin Walls wrote: >> JMX uses APIs related to the Security Mananger which are deprecated. Use of >> AccessControlContext will be removed when Security Manager is removed. >> >> Until then, updates are needed to not require setting >> -Djava.security.manager=allow to use JMX authentication. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > udpates test/jdk/javax/management/remote/mandatory/notif/policy.negative line 7: > 5: permission javax.management.MBeanPermission "[domain:type=NB,name=2]", > "addNotificationListener"; > 6: permission javax.management.MBeanPermission "*", > "removeNotificationListener"; > 7: permission javax.security.auth.AuthPermission "doAs"; I suspect that this means a doPrivileged is missing somewhere. We should not require the application to posess new permissions. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1636573141
Re: RFR: 8332071: Convert package.html files in `java.management.rmi` to package-info.java
On Sun, 19 May 2024 19:05:19 GMT, Nizar Benalla wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/package-info.java
>> line 26:
>>
>>> 24: */
>>> 25:
>>> 26: /**
>>
>> I assume you'll need to prepend each line with `*` too, which has the side
>> effect of making it appear that every line is changed but I think we just
>> need to get over that.
>
> Doing that makes git think it's a new file, rather than a rename.
> I was doing this in
> [a26ee08](https://github.com/openjdk/jdk/commit/a26ee085b5184d62a879f88f6cca6780e0e4e472)
> and removed it
LGTM - there are further potential improvements that could be made in this file
- like replacing `` with `{@code }` and `` with
`{@snippet }` but I guess that can wait until someone has the inclination and
bandwidth to do it...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19263#discussion_r1606772388
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation [v3]
On Thu, 16 May 2024 20:17:18 GMT, Kevin Walls wrote:
>> Running JConsole from a previous JDK, and attaching to jdk-23 (after
>> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
>> Management Extension (JMX) Subject Delegation feature), the MBean tab is
>> blank.
>>
>> In javax/management/remote/rmi/RMIConnectionImpl.java:
>> addNotificationListener rejects a non-null delegationSubjects array, but
>> older JDKs will send such an array. It could accept the array, and only
>> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
>> subject delegation is really happening).
>>
>> Manually testing JConsole, the MBean tab is fully populated and usable.
>
> Kevin Walls has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - add an 'also'
> - typo
Marked as reviewed by dfuchs (Reviewer).
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
line 979:
> 977: * @throws IOException if a general communication exception occurred.
> 978: * @throws UnsupportedOperationException if {@code
> delegationSubjects}
> 979: * contains any non-null values.
Suggestion:
* @throws UnsupportedOperationException if {@code delegationSubjects}
* is non-null and contains any non-null values.
-
PR Review: https://git.openjdk.org/jdk/pull/19253#pullrequestreview-2062912013
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1604707244
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Thu, 16 May 2024 10:37:36 GMT, Kevin Walls wrote: >> This shows that when SubjectDelegation was not used, a null-filled array of >> the same length as the two other arrays was expected before (in previous >> versions of the JDK where SubjectDelegation was supported, but in the case >> where it wasn't used). >> I am not suggesting to document the length requirement. The length >> requirement was enforced before and undocumented. I'm just suggesting that >> we allow null and null-filled but don't allow something (null filled array >> of wrong length) that would have been rejeceted in previous JDK versions. I >> would also suggest to check the length before the content - in case an array >> is supplied. > > Yes, completely understand. I just don't think it has any benefit. Hmmm... the spec still says: * @throws IllegalArgumentException if names or * filters is null, or if names contains * a null element, or if the three arrays do not all have the same * size. ``` - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603184648
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Thu, 16 May 2024 10:16:58 GMT, Kevin Walls wrote: >> Well my thinking was this: the fact that the jconsole tab was blank shows >> that the array may being passed. The previous code verified that all three >> arrays had the same length - so it would have failed if the array had a >> length different than the other two. So I would prefer if we kept on >> throwing in that case. In other words, we now allow and prefer `null` - but >> if non-null - we will allow a null-filled array passed by an older client >> but we should not accept something that would have been invalid then. > > That the JConsole tab was blank shows that the older RMIConnector's > addListenerWithSubject creates a new single-entry array from the given > delegationSubject (which is null) and passes it onwards. The app is not > creating the array itself., that's us. > > (also, maybe that JConsole relies on listeners in order to show a screen that > doesn't really need to depend on them, but this change is obviously about > being compatible with that) > > We all know that that is the only use case out there, the current wisdom is > that this feature is not used, nobody is creating the Subject array and > calling addListenersWithSubjects (plural) with it... > > IF we find such an app app, we are going to ignore the array unless it > contains a non-null entry. This seems safe and efficient. We are > documenting that it should be null, and it is weird to document a length > requirement for something that should be null... 8-) This shows that when SubjectDelegation was not used, a null-filled array of the same length as the two other arrays was expected before (in previous versions of the JDK where SubjectDelegation was supported, but in the case where it wasn't used). I am not suggesting to document the length requirement. The length requirement was enforced before and undocumented. I'm just suggesting that we allow null and null-filled but don't allow something (null filled array of wrong length) that would have been rejeceted in previous JDK versions. I would also suggest to check the length before the content - in case an array is supplied. - PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603087272
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 20:02:26 GMT, Kevin Walls wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
>> line 984:
>>
>>> 982: }
>>> 983: if (names.length != filters.length) {
>>> 984: final String msg = "The lengths of names and filters
>>> parameters are not same.";
>>
>> I wonder if we should check that if present, `delegationSubjects.length ==
>> names.length`?
>
> That seems pretty extreme -- it's an array we explicitly don't want and are
> going to ignore? If somebody passes a non-null member, we will throw as
> unsupported. I was thinking that was enough, hence removing the sbjs array
> to be quite certain we can't pass on any supplied Subjects.
Well my thinking was this: the fact that the jconsole tab was blank shows that
the array may being passed. The previous code verified that all three arrays
had the same length - so it would have failed if the array had a length
different than the other two. So I would prefer if we kept on throwing in that
case. In other words, we now allow and prefer `null` - but if non-null - we
will allow a null-filled array passed by an older client but we should not
accept something that would have been invalid then.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1603000965
Re: RFR: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
On Wed, 15 May 2024 16:59:59 GMT, Kevin Walls wrote:
> Running JConsole from a previous JDK, and attaching to jdk-23 (after
> [JDK-832](https://bugs.openjdk.org/browse/JDK-832): Remove the Java
> Management Extension (JMX) Subject Delegation feature), the MBean tab is
> blank.
>
> In javax/management/remote/rmi/RMIConnectionImpl.java:
> addNotificationListener rejects a non-null delegationSubjects array, but
> older JDKs will send such an array. It could accept the array, and only
> reject/throw if it contains a non-null Subject (i.e. if an attempt to use
> subject delegation is really happening).
>
> Manually testing JConsole, the MBean tab is fully populated and usable.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 984:
> 982: }
> 983: if (names.length != filters.length) {
> 984: final String msg = "The lengths of names and filters
> parameters are not same.";
I wonder if we should check that if present, `delegationSubjects.length ==
names.length`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19253#discussion_r1602032906
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with three > additional commits since the last revision: > > - Fix another typo > - Fix typo > - Add more comments Changes to jdk.net and jdk.sctp look ok. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107695217
Re: RFR: 8328619: sun/management/jmxremote/bootstrap/SSLConfigFilePermissionTest.java failed with BindException: Address already in use [v2]
On Mon, 25 Mar 2024 22:46:47 GMT, Kevin Walls wrote:
>> Test uses jdk.test.lib.Utils.getFreePort() when launching a new Java
>> command.
>> Looks like it already recognises "java.rmi.server.ExportException: Port
>> already in use: " and retries, but there is a long-standing typo in the
>> check.
>>
>> e.g.
>>
>> test output:
>> Error: Exception thrown by the agent: java.rmi.server.ExportException: Port
>> already in use: 37049; nested exception is:
>> java.net.BindException: Address already in use
>>
>> Test checks for:
>> !output.getOutput().contains("Exception thrown by the agent :
>> java.rmi.server.ExportException: Port already in use")
>>
>> Oops, we have an extra space in there. A day-one typo from JDK-7195249.
>>
>> While here, try to clarify the while loop which recognises this port
>> failure. Also add something to clarify which test(s) failed, and correct a
>> comment in test2 of AbstractFilePermissionTest.java
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> show exit code
Looks reasonable. I wish we had a way to get rid of this getFreePort() logic.
Marked as reviewed by dfuchs (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/18470#pullrequestreview-1965847115
PR Review: https://git.openjdk.org/jdk/pull/18470#pullrequestreview-1965848443
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v14]
On Thu, 14 Mar 2024 21:44:52 GMT, Kevin Walls wrote: >> The deprecated Subject Delegation feature in JMX will be removed. >> >> This was marked in JDK 21 as deprecated for removal (JDK-8298966). > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Missing code doc nit. Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18025#pullrequestreview-1938513465
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v11]
On Thu, 14 Mar 2024 11:53:11 GMT, Kevin Walls wrote: >> The deprecated Subject Delegation feature in JMX will be removed. >> >> This was marked in JDK 21 as deprecated for removal (JDK-8298966). > > Kevin Walls has updated the pull request incrementally with two additional > commits since the last revision: > > - Clarify JMXConnector equivalence comment. > - RMIConnectionImpl needs to explicity inherit the unchecked UOE. Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18025#pullrequestreview-1936454653
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v7]
On Mon, 11 Mar 2024 18:39:38 GMT, Daniel Fuchs wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
>> line 126:
>>
>>> 124: * @param className The class name of the MBean to be instantiated.
>>> 125: * @param name The object name of the MBean. May be null.
>>> 126: * @param delegationSubject No longer supported and should be null.
>>
>> I think it can be simplified to:
>>
>> Suggestion:
>>
>> * @param delegationSubject must be {@code null}.
>>
>>
>> I see no need to mention this feature no longer supported as no one is using
>> and will use this feature.
>>
>> Same comment applies to all methods that take `delegationSubject`.
>
> I was wondering if we should add a note at the class-level API documentation
> to say that subject delegation has been removed (or is no longer supported),
> and that methods will throw UOE if a non-null delegation subject is passed.
> It is otherwise strange to see a parameter that must be null. It could be
> worthwhile to mention it just once, at the class-level API.
Maybe mention there too that the delegationSubject parameter is kept for
interoperability with older remote clients.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18025#discussion_r1520271921
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v7]
On Mon, 11 Mar 2024 17:56:50 GMT, Mandy Chung wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> (C) oops
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java
> line 126:
>
>> 124: * @param className The class name of the MBean to be instantiated.
>> 125: * @param name The object name of the MBean. May be null.
>> 126: * @param delegationSubject No longer supported and should be null.
>
> I think it can be simplified to:
>
> Suggestion:
>
> * @param delegationSubject must be {@code null}.
>
>
> I see no need to mention this feature no longer supported as no one is using
> and will use this feature.
>
> Same comment applies to all methods that take `delegationSubject`.
I was wondering if we should add a note at the class-level API documentation to
say that subject delegation has been removed (or is no longer supported), and
that methods will throw UOE if a non-null delegation subject is passed. It is
otherwise strange to see a parameter that must be null. It could be worthwhile
to mention it just once, at the class-level API.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18025#discussion_r1520260840
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v7]
On Mon, 11 Mar 2024 10:09:28 GMT, Kevin Walls wrote:
>> The deprecated Subject Delegation feature in JMX will be removed.
>>
>> This was marked in JDK 21 as deprecated for removal (JDK-8298966).
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> (C) oops
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnector.java
line 618:
> 616: throw new SecurityException("Subject Delegation has been
> removed.");
> 617: }
> 618: }
Can't this constructor be removed? It's a private inner class so we should be
able to throw before calling that constructor?
test/jdk/javax/management/remote/mandatory/notif/DeadListenerTest.java line 78:
> 76: cs.start();
> 77: JMXServiceURL addr = cs.getAddress();
> 78: assertTrue("Expected no connections in new connector server",
> rmiServer.connections.isEmpty());
Changes to this file seem unrelated to the removal of the feature - could you
confirm that they were intended (no issue if they were intended).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18025#discussion_r1520108369
PR Review Comment: https://git.openjdk.org/jdk/pull/18025#discussion_r1520127466
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v5]
On Fri, 8 Mar 2024 10:20:36 GMT, Kevin Walls wrote: > Is there any value in keeping `SubjectDelegationPermission` after this > change? If so, I would mark that API deprecated for removal, so that it can > be removed in the next release or two. No issue with deprecation. I guess it can be removed once `java.security.Policy` is removed? - PR Comment: https://git.openjdk.org/jdk/pull/18025#issuecomment-1988693902
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 5 Mar 2024 19:53:46 GMT, Weijun Wang wrote: >> Subject is stored in the RMIConnectionImpl: >> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java >> >> (That is complicated by SubjectDelegation, which we deprecated for removal. >> I have the PR out to remove it: >> https://github.com/openjdk/jdk/pull/18025 ) >> >> makeClient in RMIJRMPServerImpl creates RMIConnectionImpl >> >> ..and RMIServerImpl.java has a doNewClient method calling that. This is >> what takes a Credentials Object and deals withJMXAuthenticator to get an >> authenticated Subject. None of this requires the SM. > > I see that in `RMIConnectionImpl.java` it is stored inside an > `AccessControlContext`, and there are `doPrivileged` calls on this ACC to > pass the subject into an action. So, even if there might be no SM but the > subject will never be bound to a thread using a scoped value. > > I’ll revert the change then, and this code must have SM allowed to run > correctly. If user runs it with SM disabled, at least they will see an UOE > instead of letting `current()` silently returns a null. > > Ultimately, if we want it working after SM, we should update > `RMIConnectionImpl` and rewrite the ACC-related code to using > `Subject.callAs`. Yes - the JMX implementation stores and retrieve subjects in the AccessControlContext and then uses doPrivileged. Subject.doAs is not used in the JMX implementation. There are two different uses of Subject in JMX: 1. one is a simplified role-based authentication/authorization at the default agent level. 2. The other one is a permission check where different subjects can be granted different privileges in the policy file. The latter will go away since the SM is going away, but needs to be preserved until then. The former we want to keep and needs to keep working (by changing the code to use callAs) even after the SM is gone. Subject delegation allows an authenticated subject (1. above) to perform actions on behalf of a delegation subject, where the privileged granted by 2. are the intersection of the privileges of the two subjects. @kevinjwalls is currently working on removing subject delegation. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1515896397
Re: RFR: 8326666: Remove the Java Management Extension (JMX) Subject Delegation feature [v2]
On Mon, 4 Mar 2024 13:24:05 GMT, Kevin Walls wrote: >> The deprecated Subject Delegation feature in JMX will be removed. >> >> This was marked in JDK 21 as deprecated for removal (JDK-8298966). > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > RMIConnection comments update In addition to Alan's remarks I believe we need new tests to verify the new behavior. - PR Comment: https://git.openjdk.org/jdk/pull/18025#issuecomment-1976841428
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. Approving the sun.net changes. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17789#pullrequestreview-1875818676
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. changes to sun/net content-types.properties look OK - PR Comment: https://git.openjdk.org/jdk/pull/17789#issuecomment-1935996382
Re: RFR: 8325109: Sort method modifiers in canonical order
On Thu, 1 Feb 2024 11:57:04 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8324053](https://bugs.openjdk.org/browse/JDK-8324053). I have run the > bin/blessed-modifier-order.sh on the entire code base, and manually checked > the result. I have reverted all but these trivial and uncontroversial changes. > > Almost all of these are about moving `static` to its proper position; a few > do not involve `static` but instead puts `final` or `abstract` in the correct > place. > > I have deliberately stayed away from `default` in this PR, since they should > probably get a more thorough treatment, see > https://github.com/openjdk/jdk/pull/17468#pullrequestreview-1829238473. Changes to IPAdressUtil look fine. I eyeballed the rest and didn't spot any issue. - PR Review: https://git.openjdk.org/jdk/pull/17670#pullrequestreview-1856519410
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
On Tue, 30 Jan 2024 13:53:37 GMT, Daniel Fuchs wrote:
>> This code change adds an alternative implementation of user-based
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
>> Depending on if the Security Manager is allowed, the methods store the
>> current subject differently. See the spec change in the `Subject.java` file
>> for details. When the Security Manager APIs are finally removed in a future
>> release, this new implementation will be only implementation for these
>> methods.
>>
>> One major change in the new implementation is that `Subject.getSubject`
>> always throws an `UnsupportedOperationException` since it has an
>> `AccessControlContext` argument but the current subject is no longer
>> associated with an `AccessControlContext` object.
>>
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to
>> `current` and `callAs`. If the user application is simply calling
>> `getSubject(AccessController.getContext())`, then switching to `current()`
>> would work. If the `AccessControlContext` argument is retrieved from an
>> earlier `getContext()` call and the associated subject might be different
>> from that of the current `AccessControlContext`, then instead of storing the
>> previous `AccessControlContext` object and passing it into `getSubject` to
>> get the "previous" subject, the application should store the `current()`
>> return value directly.
>
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 349:
>
>> 347: @SuppressWarnings("removal")
>> 348: private Subject getSubject() {
>> 349: return Subject.current();
>
> Since `Subject::current` is not deprecated the annotation at line 347 above
> should be removed.
OK - things seem to be a bit convoluted here and some pieces might be missing.
I suspect that what needs to be done is more complicated:
`RMIConnectionImpl` sets up an ACC and calls doPrivileged with that ACC, on the
assumption that the subject is tied to the ACC and it can be retrieved down the
road from the ACC. `RMIConnectionImpl` has the subject, and the delegation
subject too.
So for `Subject::current` to work here, shouldn't
`RMIConnectionImpl::doPrivilegedOperation` use `Subject::callAs` when the
security manager is disallowed?
It seems that when `Subject::current` is used, some analysis should be done to
verify where the Subject is supposed to come from - that is - how the caller is
expecting the subject to reach the callee.
Typically, JMX doesn't use `Subject::doAs` but ties a `Subject` to an
`AccessControlContext` and uses `doPrivileged` instead.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1471308151
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang wrote: > This code change adds an alternative implementation of user-based > authorization `Subject` APIs that doesn't depend on Security Manager APIs. > Depending on if the Security Manager is allowed, the methods store the > current subject differently. See the spec change in the `Subject.java` file > for details. When the Security Manager APIs are finally removed in a future > release, this new implementation will be only implementation for these > methods. > > One major change in the new implementation is that `Subject.getSubject` > always throws an `UnsupportedOperationException` since it has an > `AccessControlContext` argument but the current subject is no longer > associated with an `AccessControlContext` object. > > Now it's the time to migrate from the `getSubject` and `doAs` methods to > `current` and `callAs`. If the user application is simply calling > `getSubject(AccessController.getContext())`, then switching to `current()` > would work. If the `AccessControlContext` argument is retrieved from an > earlier `getContext()` call and the associated subject might be different > from that of the current `AccessControlContext`, then instead of storing the > previous `AccessControlContext` object and passing it into `getSubject` to > get the "previous" subject, the application should store the `current()` > return value directly. Changes requested by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17472#pullrequestreview-1851409950
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs
On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang wrote:
> This code change adds an alternative implementation of user-based
> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
> Depending on if the Security Manager is allowed, the methods store the
> current subject differently. See the spec change in the `Subject.java` file
> for details. When the Security Manager APIs are finally removed in a future
> release, this new implementation will be only implementation for these
> methods.
>
> One major change in the new implementation is that `Subject.getSubject`
> always throws an `UnsupportedOperationException` since it has an
> `AccessControlContext` argument but the current subject is no longer
> associated with an `AccessControlContext` object.
>
> Now it's the time to migrate from the `getSubject` and `doAs` methods to
> `current` and `callAs`. If the user application is simply calling
> `getSubject(AccessController.getContext())`, then switching to `current()`
> would work. If the `AccessControlContext` argument is retrieved from an
> earlier `getContext()` call and the associated subject might be different
> from that of the current `AccessControlContext`, then instead of storing the
> previous `AccessControlContext` object and passing it into `getSubject` to
> get the "previous" subject, the application should store the `current()`
> return value directly.
src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
line 349:
> 347: @SuppressWarnings("removal")
> 348: private Subject getSubject() {
> 349: return Subject.current();
Since `Subject::current` is not deprecated the annotation at line 347 above
should be removed.
src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
line 307:
> 305: AccessController.doPrivileged(new PrivilegedAction<>() {
> 306: public Subject run() {
> 307: return Subject.current();
Is the `doPrivileged` still needed here? Is there a chance that
`Subject.current()` will throw a `SecurityException`, or return a different
result if a security manager is present and `doPrivileged` is not used?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1471257982
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1471263581
Re: RFR: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature [v4]
On Thu, 11 Jan 2024 20:59:05 GMT, Kevin Walls wrote: >> 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 > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Test update using URLClassLoader MBeans, more like original tests 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. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16363#pullrequestreview-1818296233
Re: RFR: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature [v3]
On Thu, 11 Jan 2024 11:31:07 GMT, Kevin Walls wrote: >> Yes, that's the whole point of the test. Without using an MBean which is a >> ClassLoader the fix cannot be tested. >> 1. 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. >> 2. 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). >> 3. 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. >> 4. you then keep a weak reference on the _LoaderMBean_, and unregister both >> MBeans. >> 5. 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/16363#discussion_r1448772336
Re: RFR: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature [v3]
On Wed, 10 Jan 2024 16:50:22 GMT, Kevin Walls wrote: >> 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. 1. 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. 2. 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). 3. 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. 4. you then keep a weak reference on the _LoaderMBean_, and unregister both MBeans. 5. 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). - PR Review Comment: https://git.openjdk.org/jdk/pull/16363#discussion_r1447675022
Re: RFR: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature [v3]
On Wed, 10 Jan 2024 15:15:36 GMT, Kevin Walls wrote:
>> test/jdk/javax/management/Introspector/ClassLeakTest.java line 55:
>>
>>> 53: ObjectName testName = new ObjectName("x:name=Test");
>>> 54: Test mbean = new Test();
>>> 55: 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).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16363#discussion_r1447605593
Re: RFR: 8318707: Remove the Java Management Extension (JMX) Management Applet (m-let) feature [v3]
On Thu, 4 Jan 2024 10:48:38 GMT, Kevin Walls wrote:
>> 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
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove unnecessary MLET_ Properties
test/jdk/javax/management/Introspector/ClassLeakTest.java line 55:
> 53: ObjectName testName = new ObjectName("x:name=Test");
> 54: Test mbean = new Test();
> 55: 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.
test/jdk/javax/management/mxbean/MXBeanLoadingTest1.java line 85:
> 83: MBeanServer mbs = MBeanServerFactory.createMBeanServer();
> 84: ObjectName testName = new ObjectName("x:type=test");
> 85: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16363#discussion_r1447419444
PR Review Comment: https://git.openjdk.org/jdk/pull/16363#discussion_r1447428031
Re: RFR: 8321729: Remove 'orb' field in RMIConnector
On Mon, 11 Dec 2023 12:50:38 GMT, Kevin Walls wrote: > Tidyup change, looks like this field was not removed when IIOP was removed > from RMIConnector. > > The field is not required for interoperability: > with the field removed, I can still connect an older JMX client to a running > app with the updated JDK. > > Since the JDK9 change https://hg.openjdk.org/jdk9/jdk9/jdk/rev/09daaf1e4c53 > did not change serialVersionUID, this update does not either. > > Tests continue to pass, e.g. > javax/management (including remote which has RMI tests) > jdk/com/sun/management Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17055#pullrequestreview-1775807650
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Tue, 28 Nov 2023 16:21:29 GMT, Kevin Walls wrote: > JMX RMI Connections should use a timeout on the Socket connect call by > default. > > JMX Connections use RMI and some connection failures never terminate. The > hang described in 8316649 is hard to reproduce manually: the description says > it can be caused by a firewall that never returns a response. > > Changing the base RMI implementation may not be desirable at this time. > > JMX can use a new ClientSocketFactory for RMI which implements the connect > timeout, which can recognise a new JMX-specific property > `com.sun.management.jmxremote.rmi.tcpConnectTimeout` > (named like the existing com.sun.management.jmxremote... properties) > > Defaulting to a 1 minute timeout on connect has no effect on existing tests, > and should go unnoticed unless there really is a significant connection > delay. Specifying zero for the new System Property will use the old technique > of a connect with no timeout. > > This can be tested, but it is not realistically usable: although specifying a > 1 millisecond timeout will often fail (as expected/desired for the test), it > will very often pass as the connection happens immediately. I don't think that's a good idea. AFAIK the RMIClientSocketFactory is serialized and sent by the server to the client. This means, unless I'm mistaken, that you will not be able to use JDK 21 JConsole to connect to a JDK 22 VM. Try it :-) If you chose to go this route then use of the factory should be an opt-in. - PR Comment: https://git.openjdk.org/jdk/pull/16856#issuecomment-1830389934
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Mon, 27 Nov 2023 15:00:54 GMT, Daniel Fuchs wrote: >> On the Property name: there is an existing System Property >> "sun.rmi.transport.connectionTimeout" which is a 15-second idle timeout. >> Having a "connectionTimeout" and this new one as "connectTimeout" could be >> confusing, even in different but very similar package names, hence naming >> this "initialConnectTimeout". >> (So "initial" to signal that it's the initial connect of a socket, different >> to the existing "connectionTimeout".) >> It does not seem like the kind of system property a user would expect to be >> read again on every usage, I think, let me know if you see that as a problem. >> >> I am hoping that as we already have various properties fetched in the same >> manner, we don't see the need to pursue a test that the value is fetched and >> passed on. > > Have you considered naming it "socketConnectTimeout" instead? Or possibly > "tcpConnectTimeout"? If you named it "tcpConnectTimeout" then you could name the (future) property for [JDK-8320703](https://bugs.openjdk.org/browse/JDK-8320703: JMX SSL RMI Socket connection timeout cannot be changed) "tlsConnectTimeout"... - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1406309820
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Mon, 27 Nov 2023 14:35:07 GMT, Kevin Walls wrote: >> wrt to the property name initialConnectTimeout might infer that it is an >> initial value which can be subsequentually modified, but that is not >> possible. As such, would sun.rmi.transport.tcp.connectTimeout be more >> appropriate -- dropping the initial ? >> >> If the connectTimeout initialization code was placed in a static method, it >> could be directly unit testable :-) (if such a test was desirable for >> coverage completeness) > > On the Property name: there is an existing System Property > "sun.rmi.transport.connectionTimeout" which is a 15-second idle timeout. > Having a "connectionTimeout" and this new one as "connectTimeout" could be > confusing, even in different but very similar package names, hence naming > this "initialConnectTimeout". > (So "initial" to signal that it's the initial connect of a socket, different > to the existing "connectionTimeout".) > It does not seem like the kind of system property a user would expect to be > read again on every usage, I think, let me know if you see that as a problem. > > I am hoping that as we already have various properties fetched in the same > manner, we don't see the need to pursue a test that the value is fetched and > passed on. Have you considered naming it "socketConnectTimeout" instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1406293632
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Fri, 24 Nov 2023 12:13:56 GMT, Kevin Walls wrote: >> (Look for socket factories in the module `jdk.management.agent`) > > OK yes, we also have: > java.rmi/share/classes/javax/rmi/ssl/SslRMIClientSocketFactory.java > with its own createSocket(String host, int port) method. This is used if we > use JMX over SSL. > > So SslRMIClientSocketFactory could specifically implement the connect timeout. > > Next q, should it? 8-) > > The reported hang and those I have seen in testing have only been in: > sun.rmi.transport.tcp.TCPDirectSocketFactory.createSocket calling Socket.init. > > javax/rmi/ssl/SslRMIClientSocketFactory.java reads some properties named > "javax.rmi.ssl.client" > so it would be odd for it to read > "sun.rmi.transport.tcp.initialConnectTimeout" I was proposing here. > > It could implement "javax.rmi.ssl.client.initialConnectTimeout", or we could > leave SSL alone for now, possibly handling it in a separate issue if it's > wanted. OK - sounds good. Meanwhile I had a look at the custom RMI Socket Factories used by the JMX Agent, and these are actually RMIServerSocketFactories, so having a timeout for connect there probably makes no sense. - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1404331875
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 17:17:34 GMT, Daniel Fuchs wrote: >>> I see Integer.getInteger handles the obvious Exceptions, so specifying a >>> strange value does not immediately break us. >> >> Oh - right. It's `Integer.getInteger`, not `Integer.parseInt` Ok, then - I >> withdraw my first comment :-) > >> This is a stack from a test I was experimenting with, when it did see the >> timeout: > > Ah - that's for testing with a particular test. So the question now is: > > Should the RMISocketFactories provided / implemented in the JMX > implementation - such as those used by the JMX default agent, also honour > this system property? (Look for socket factories in the module `jdk.management.agent`) - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403622189
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 17:14:52 GMT, Daniel Fuchs wrote: >>> IIRC, the default agent uses / may use its own socket factories - are we >>> still coming here even with those factories? >> >> We do get here, yes (not saying you can't customise your way out of getting >> here). This is a stack from a test I was experimenting with, when it did >> see the timeout: >> >> >> -System.out:(4/132)-- >> JMX URL = service:jmx:rmi://x >> expectTimeout = true >> sun.rmi.transport.tcp.initialConnectTimeout = 1 >> Test passed >> --System.err:(24/1791)-- >> java.rmi.ConnectIOException: Exception creating connection to: x.x.x.x; >> nested exception is: >> java.net.SocketTimeoutException >> at >> java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:637) >> at >> java.rmi/sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java:217) >> at >> java.rmi/sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java:204) >> at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:134) >> at >> java.management.rmi/javax.management.remote.rmi.RMIServerImpl_Stub.newClient(RMIServerImpl_Stub.java:85) >> at >> java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java:2106) >> at >> java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:321) >> at >> java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) >> at >> java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:229) >> at RMIConnectionTimeoutTest.main(RMIConnectionTimeoutTest.java:65) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) >> at java.base/java.lang.reflect.Method.invoke(Method.java:580) >> at >> com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) >> at java.base/java.lang.Thread.run(Thread.java:1570) >> Caused by: java.net.SocketTimeoutException >> at >> java.base/java.net.SocksSocketImpl.remainingMillis(SocksSocketImpl.java:112) >> at >> java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327) >> at java.base/java.net.Socket.connect(Socket.java:752) >> at >> java.rmi/sun.rmi.transport.tcp.TCPDirectSocketFactory.createSocket(TCPDirectSocketFactory.java:57) >> at >> java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:619) >> ... 13 more >> STATUS:Passed. > >> I see Integer.getInteger handles the obvious Exceptions, so specifying a >> strange value does not immediately break us. > > Oh - right. It's `Integer.getInteger`, not `Integer.parseInt` Ok, then - I > withdraw my first comment :-) > This is a stack from a test I was experimenting with, when it did see the > timeout: Ah - that's for testing with a particular test. So the question now is: Should the RMISocketFactories provided / implemented in the JMX implementation - such as those used by the JMX default agent, also honour this system property? - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403620057
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 12:43:33 GMT, Kevin Walls wrote: >>> An exception here will prevent the class from being initialized... >> >> Maybe we can break it, but this new property is following the same pattern I >> think as the neighbouring file >> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java when it >> reads the system props. I see Integer.getInteger handles the obvious >> Exceptions, so specifying a strange value does not immediately break us. >> >> If there's more protection needed then we have various other places to apply >> it that might need separate follow-up if you think there's a case? > >> IIRC, the default agent uses / may use its own socket factories - are we >> still coming here even with those factories? > > We do get here, yes (not saying you can't customise your way out of getting > here). This is a stack from a test I was experimenting with, when it did see > the timeout: > > > -System.out:(4/132)-- > JMX URL = service:jmx:rmi://x > expectTimeout = true > sun.rmi.transport.tcp.initialConnectTimeout = 1 > Test passed > --System.err:(24/1791)-- > java.rmi.ConnectIOException: Exception creating connection to: x.x.x.x; > nested exception is: > java.net.SocketTimeoutException > at > java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:637) > at > java.rmi/sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java:217) > at > java.rmi/sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java:204) > at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:134) > at > java.management.rmi/javax.management.remote.rmi.RMIServerImpl_Stub.newClient(RMIServerImpl_Stub.java:85) > at > java.management.rmi/javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java:2106) > at > java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:321) > at > java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270) > at > java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:229) > at RMIConnectionTimeoutTest.main(RMIConnectionTimeoutTest.java:65) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:580) > at > com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138) > at java.base/java.lang.Thread.run(Thread.java:1570) > Caused by: java.net.SocketTimeoutException > at > java.base/java.net.SocksSocketImpl.remainingMillis(SocksSocketImpl.java:112) > at > java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:327) > at java.base/java.net.Socket.connect(Socket.java:752) > at > java.rmi/sun.rmi.transport.tcp.TCPDirectSocketFactory.createSocket(TCPDirectSocketFactory.java:57) > at > java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:619) > ... 13 more > STATUS:Passed. > I see Integer.getInteger handles the obvious Exceptions, so specifying a > strange value does not immediately break us. Oh - right. It's `Integer.getInteger`, not `Integer.parseInt` Ok, then - I withdraw my first comment :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403618437
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Tue, 21 Nov 2023 17:57:32 GMT, Kevin Walls wrote:
> RMI Connections (in general) should use a timeout on the Socket connect call
> by default.
>
> JMX connections use RMI and some connection failures never terminate. The
> hang described in 8316649 is hard to reproduce manually: the description says
> it can be caused by a firewall that never returns a response.
>
> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> has other timeouts but nothing to control the initial Socket connect.
>
> Defaulting to a 1 minute timeout on connect has no effect on existing tests
> for RMI and JMX, and should go unnoticed in applications unless there really
> is a significant connection delay. Specifying zero for the new System
> Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path
> of a connect with no timeout.
>
> I have a test, but it is not realistically usable: although specifying a 1
> millisecond timeout will often fail (as expected/desired for the test), it
> will often pass as the connection happens immediately.
src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPDirectSocketFactory.java
line 46:
> 44: private static final int connectTimeout =// default 1 minute
> 45: AccessController.doPrivileged((PrivilegedAction) () ->
> 46:
> Integer.getInteger("sun.rmi.transport.tcp.initialConnectTimeout", 60 *
> 1000).intValue());
An exception here will prevent the class from being initialized, any subsequent
attempts to use the class will then produce hard to diagnose error. If the
class is not loaded by the main thread, such exception/error could be swallowed
unless an uncaught exception handler was registered. I would suggest
implementing the logic in a static block, catch any exception and revert to
default behaviour (i.e. 0) if the value supplied for the property is not an
integer, or not a valid integer, etc...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403258620
Re: RFR: 8316649: JMX connection timeout cannot be changed and uses the default of 0 (infinite)
On Thu, 23 Nov 2023 11:35:11 GMT, Daniel Fuchs wrote:
>> RMI Connections (in general) should use a timeout on the Socket connect call
>> by default.
>>
>> JMX connections use RMI and some connection failures never terminate. The
>> hang described in 8316649 is hard to reproduce manually: the description
>> says it can be caused by a firewall that never returns a response.
>>
>> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
>> has other timeouts but nothing to control the initial Socket connect.
>>
>> Defaulting to a 1 minute timeout on connect has no effect on existing tests
>> for RMI and JMX, and should go unnoticed in applications unless there really
>> is a significant connection delay. Specifying zero for the new System
>> Property sun.rmi.transport.tcp.initialConnectTimeout uses the old code path
>> of a connect with no timeout.
>>
>> I have a test, but it is not realistically usable: although specifying a 1
>> millisecond timeout will often fail (as expected/desired for the test), it
>> will often pass as the connection happens immediately.
>
> src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPDirectSocketFactory.java
> line 46:
>
>> 44: private static final int connectTimeout =// default 1 minute
>> 45: AccessController.doPrivileged((PrivilegedAction) () ->
>> 46:
>> Integer.getInteger("sun.rmi.transport.tcp.initialConnectTimeout", 60 *
>> 1000).intValue());
>
> An exception here will prevent the class from being initialized, any
> subsequent attempts to use the class will then produce hard to diagnose
> error. If the class is not loaded by the main thread, such exception/error
> could be swallowed unless an uncaught exception handler was registered. I
> would suggest implementing the logic in a static block, catch any exception
> and revert to default behaviour (i.e. 0) if the value supplied for the
> property is not an integer, or not a valid integer, etc...
IIRC, the default agent uses / may use its own socket factories - are we still
coming here even with those factories?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16771#discussion_r1403260460
Re: RFR: 8319465: Typos in javadoc of com.sun.management.OperatingSystemMXBean methods
On Mon, 6 Nov 2023 06:55:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this PR which fixes the typos in the method > javadocs of com.sun.management.OperatingSystemMXBean? > > As noted in https://bugs.openjdk.org/browse/JDK-8319465, this PR replaces the > word "betweens" by "between" Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16516#pullrequestreview-1715042344
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. jmx, jndi, and net changes LGTM - PR Review: https://git.openjdk.org/jdk/pull/15573#pullrequestreview-1613147541
Re: RFR: 8313997: The jdi/ListeningConnector/startListening/startlis001 may fail if the hosts file is modified
On Wed, 9 Aug 2023 07:46:58 GMT, Sergey Bylokhov wrote: > The test uses this code to create a list of valid addresses for the localhost: > > String hostname = "localhost"; > List validAddresses = new LinkedList<>(); > validAddresses.add(hostname); > Arrays.stream(InetAddress.getAllByName(hostname)) > .forEach(address -> > validAddresses.add(address.getHostAddress())); > > It does not work properly if the custom name is set for the 127.0.01 via > /etc/hosts file. In that case, the address returned by the "startListen()" > will be "SomeCustomName:port", for example we can return it here: > https://github.com/openjdk/jdk/blob/360f65d7b15b327e2f160c42f318945cc6548bda/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java#L82 I would recommend fixing the /etc/hosts file in such cases, rather than fixing the test to listen on a potential external / non loopback address (which might also cause the test to fail on other configurations). - PR Comment: https://git.openjdk.org/jdk/pull/15203#issuecomment-1672919992
Re: [jdk21] RFR: 8300937: Update nroff pages in JDK 21 before RC
On Mon, 31 Jul 2023 08:33:07 GMT, David Holmes wrote: > Main changes are to use 21 instead of 21-ea. In addition the following files > contain additional updates from the closed sources: > > - src/java.base/share/man/java.1 > > [JDK-8273131](https://bugs.openjdk.org/browse/JDK-8273131): Update the java > manpage markdown source for JFR filename expansion > [JDK-8221633](https://bugs.openjdk.org/browse/JDK-8221633): > StartFlightRecording: consider moving mention of multiple comma-separated > parameters to the front > > There are also some formatting differences related to: > > [JDK-8309670](https://bugs.openjdk.org/browse/JDK-8309670): java -help > output for --module-path / -p is incomplete > > Likely a different version of pandoc was used. > > - src/java.base/share/man/keytool.1 > > [JDK-8290626](https://bugs.openjdk.org/browse/JDK-8290626): keytool manpage > contains a special character > > - src/jdk.compiler/share/man/javac.1 > > [JDK-8308456](https://bugs.openjdk.org/browse/JDK-8308456): Update javac > tool page for -proc:full > > also some formatting differences. > > - src/jdk.jartool/share/man/jarsigner.1 > > [JDK-8303928](https://bugs.openjdk.org/browse/JDK-8303928): Update jarsigner > man page after [JDK-8303410](https://bugs.openjdk.org/browse/JDK-8303410) > > - src/jdk.jcmd/share/man/jcmd.1 > > [JDK-8273131](https://bugs.openjdk.org/browse/JDK-8273131): Update the java > manpage markdown source for JFR filename expansion > > - src/jdk.jdeps/share/man/jdeps.1 > > [JDK-8301207](https://bugs.openjdk.org/browse/JDK-8301207): (jdeps) > Deprecate jdeps -profile option > > - src/jdk.jfr/share/man/jfr.1 > > Formatting changes. > > - src/jdk.jshell/share/man/jshell.1 > > [JDK-8308988](https://bugs.openjdk.org/browse/JDK-8308988): Update JShell > manpage to include TOOLING description Changes to jwebserver.1 look good to me. - PR Comment: https://git.openjdk.org/jdk21/pull/151#issuecomment-1658469731
Re: RFR: 8307244: Remove redundant class RMIIIOPServerImpl
On Tue, 2 May 2023 17:57:14 GMT, Kevin Walls wrote: > Removal of class, looks like it was missed in the JDK9 removal of RMIIIOP. > This class is not referenced by other classes or tests. Marked as reviewed by dfuchs (Reviewer). Looks good to me. I probably wouldn't have bothered with removing the example of IIOP JMXServiceURL as arguably functional implementations of that might still exist (as long as they don't extend RMIIIOPServerImpl, which is not a requirement). The constructor of RMIIIOPServerImpl throws UnsupportedOperationException unconditionally, so there can't exist any functional subclasses of that class that could be instantiated on Java versions posterior to Java 9. As such removing that class from the public API sounds reasonable. - PR Review: https://git.openjdk.org/jdk/pull/13758#pullrequestreview-1410697538 PR Comment: https://git.openjdk.org/jdk/pull/13758#issuecomment-1532865857
Re: RFR: 8298966: Deprecate JMX Subject Delegation and the method JMXConnector.getMBeanServerConnection(Subject) for removal. [v2]
On Fri, 3 Mar 2023 11:46:49 GMT, Kevin Walls wrote: >> Deprecate the Java Management Extension (JMX) Subject Delegation feature for >> removal in a future release. >> >> Given no known usage, there is no replacement feature for JMX Subject >> Delegation. >> >> CSR is https://bugs.openjdk.org/browse/JDK-8298967 > > Kevin Walls has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - deprecation text update > - Revert "RMIConnection throw comments" > >This reverts commit aceb4fe44189245ac702f0c74c2bb1100a6d17fa. > - Merge remote-tracking branch 'upstream/master' into > Deprecate_SubjectDelegation > - RMIConnection throw comments > - 8298966: Deprecate JMX Subject Delegation and the method > JMXConnector.getMBeanServerConnection(Subject) for removal. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11880
Re: RFR: 8299234: JMX Repository.query performance [v6]
On Fri, 17 Feb 2023 18:25:10 GMT, Alexey Bakhtin wrote: >> Please find a patch to improve JMX Repository.query performance >> >> Using ObjectName.apply() allows significantly decrease memory usage and the >> number of GC cycles: >> Before: >> >> $ java test 100 100 >> Test PASSED in 8943169791 ns. >> GC: G1 Young Generation getCollectionCount()=177 getCollectionTime()=118 >> >> >> After: >> >> $ java test 100 100 >> Test PASSED in 4808213917 ns. >> GC: G1 Young Generation getCollectionCount()=88 getCollectionTime()=53 >> >> Private ObjectName.matchDomains() method is also updated to minimize >> unnecessary memory allocation. >> >> All javax/management jtreg tests passed successfully. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Add braces This last version LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11758
Re: RFR: 8299234: JMX Repository.query performance [v4]
On Mon, 23 Jan 2023 20:25:17 GMT, Alexey Bakhtin wrote: >> Please find a patch to improve JMX Repository.query performance >> >> Using ObjectName.apply() allows significantly decrease memory usage and the >> number of GC cycles: >> Before: >> >> $ java test 100 100 >> Test PASSED in 8943169791 ns. >> GC: G1 Young Generation getCollectionCount()=177 getCollectionTime()=118 >> >> >> After: >> >> $ java test 100 100 >> Test PASSED in 4808213917 ns. >> GC: G1 Young Generation getCollectionCount()=88 getCollectionTime()=53 >> >> Private ObjectName.matchDomains() method is also updated to minimize >> unnecessary memory allocation. >> >> All javax/management jtreg tests passed successfully. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Revert changes in the ObjectName I don't see an obvious issue with the proposed changes. Do you have a JMH benchmark to prove the new code is faster? Also please obtain a review from a maintainer of the servicieability area before integrating. - PR: https://git.openjdk.org/jdk/pull/11758
Re: RFR: 8299234: JMX Repository.query performance [v3]
On Wed, 11 Jan 2023 14:09:21 GMT, Alexey Bakhtin wrote: >> Please find a patch to improve JMX Repository.query performance >> >> Using ObjectName.apply() allows significantly decrease memory usage and the >> number of GC cycles: >> Before: >> >> $ java test 100 100 >> Test PASSED in 8943169791 ns. >> GC: G1 Young Generation getCollectionCount()=177 getCollectionTime()=118 >> >> >> After: >> >> $ java test 100 100 >> Test PASSED in 4808213917 ns. >> GC: G1 Young Generation getCollectionCount()=88 getCollectionTime()=53 >> >> Private ObjectName.matchDomains() method is also updated to minimize >> unnecessary memory allocation. >> >> All javax/management jtreg tests passed successfully. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Fix parameters order for Util.wildmatch Hi Alexey - you have addressed most of my concerns but let me reinstate that the changes to ObjectName are observable by subclasses and thus will require a CSR. As such this fix may also not be a good candidate for backport (if that was your intention), as it comes with a potential regression risk. - PR: https://git.openjdk.org/jdk/pull/11758
Re: RFR: JDK-8300357: Use generalized see and link tags in java.management [v3]
On Wed, 18 Jan 2023 19:13:41 GMT, Joe Darcy wrote: >> Use new javadoc capabilities courtesy JDK-8200337 to have >> more-readable-in-javadoc-source links to anchors in several management >> types. Analogous change is out for review in core libs, JDK-8300133. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Improve link target per code review comments. LGTM now. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/12058
Re: [jdk20] RFR: 8299891: JMX ObjectInputFilter additional classes needed [v2]
On Wed, 18 Jan 2023 17:56:43 GMT, Kevin Walls wrote: >> The default setting for the ObjectInputFilter for JMX, introduced in jdk20, >> is too restrictive. >> javax.management.Attribute and AttributeList classes are also needed, and >> Query related classes. >> >> There are a number of Query-relate classes, so adding javax.management.* is >> appropriate otherwise the list becomes hard to manage. This is a * and not >> a ** which would mean all subpackages, so the openmean subpackage stays in >> the list. > > Kevin Walls has updated the pull request incrementally with two additional > commits since the last revision: > > - filter properties update: more classes logged as ALLOWED during wider > testing > - Add a Notification test Thanks for adding the new testcases, especially WRT to notifications. Your filter might be a little wider than strictly required but it looks like a good first step, and is obviously better than no filter. I am approving on the condition that all JMX (and JCK?) tests are stable passing. Please obtain approval from a maintainer of this area before pushing. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk20/pull/97
Re: RFR: JDK-8300357: Use generalized see and link tags in java.management
On Wed, 18 Jan 2023 00:37:05 GMT, Joe Darcy wrote:
> Use new javadoc capabilities courtesy JDK-8200337 to have
> more-readable-in-javadoc-source links to anchors in several management types.
> Analogous change is out for review in core libs, JDK-8300133.
src/java.management/share/classes/javax/management/remote/JMXConnectorFactory.java
line 140:
> 138: * implementation may choose to find providers by other means. For
> 139: * example, it may support {@linkplain
> 140: * java.base/java.util.ServiceLoader##developing-service-providers
> service providers},
is `java.base/` needed here? I am a bit surprised - it's first time I see a
module name used in a regular link target.
src/java.management/share/classes/javax/management/remote/JMXConnectorServerFactory.java
line 129:
> 127: * MalformedURLException if there is none. An
> 128: * implementation may choose to find providers by other means. For
> 129: * example, it may support {@linkplain
Same remark regarding `java.base/` prefix.
-
PR: https://git.openjdk.org/jdk/pull/12058
Re: RFR: 8300032: DwarfParser resource leak
On Thu, 12 Jan 2023 12:08:51 GMT, Daniel Jeliński wrote: > Please review this fix for DwarfParser cleaner. > > The original code registered the cleaner using a lambda that captured a > reference to the parser object; as a result, the object was never GCed, and > the cleaner never ran. > > In this version I moved the lambda creation to a static method, so that it > can't capture a reference to the parser. > > Additionally, the new code uses a static cleaner; the cleaner object creation > and cleanup are heavy operations (every cleaner comes with its own thread), > and it's preferable to use a single shared cleaner where possible. > > I verified manually that the native `destroyDwarfContext` method is called > after this fix. No new automated test. Existing tier1-2 tests continue to > pass. Though I am not a usual reviewer for this area I believe the logic of your change is good. Please obtain the review from a maintainer of the area before integrating. - PR: https://git.openjdk.org/jdk/pull/11965
Re: [jdk20] RFR: 8299891: JMX ObjectInputFilter additional classes needed
On Wed, 11 Jan 2023 09:40:11 GMT, Kevin Walls wrote: > The default setting for the ObjectInputFilter for JMX, introduced in jdk20, > is too restrictive. > javax.management.Attribute and AttributeList classes are also needed, and > Query related classes. > > There are a number of Query-relate classes, so adding javax.management.* is > appropriate otherwise the list becomes hard to manage. This is a * and not a > ** which would mean all subpackages, so the openmean subpackage stays in the > list. I believe it would be good to add a test method that registers for notifications using a notification filter. Especially attribute change notification, possibly MBean registration too. - PR: https://git.openjdk.org/jdk20/pull/97
Re: RFR: 8299234: JMX Repository.query performance [v2]
On Wed, 11 Jan 2023 11:29:39 GMT, Alexey Bakhtin wrote: >> Please find a patch to improve JMX Repository.query performance >> >> Using ObjectName.apply() allows significantly decrease memory usage and the >> number of GC cycles: >> Before: >> >> $ java test 100 100 >> Test PASSED in 8943169791 ns. >> GC: G1 Young Generation getCollectionCount()=177 getCollectionTime()=118 >> >> >> After: >> >> $ java test 100 100 >> Test PASSED in 4808213917 ns. >> GC: G1 Young Generation getCollectionCount()=88 getCollectionTime()=53 >> >> Private ObjectName.matchDomains() method is also updated to minimize >> unnecessary memory allocation. >> >> All javax/management jtreg tests passed successfully. > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Use copy of the ObjectName for matching Using `ObjectName::getInstance` for the defensive copy is the right choice since it will only make a copy if the argument is a subclass. That looks good. I see your point about using `_canonical_name`. Technically this is still an observable change of behaviour - so it might be preferable to file a CSR if you want to proceed with the changes made to `ObjectName`. Regardless of that these changes seem to be introducing a bug (see my other comment inline). src/java.management/share/classes/javax/management/ObjectName.java line 2024: > 2022: // The other ObjectName is the string. > 2023: return Util.wildmatch(name._canonicalName, _canonicalName, > 2024:0, 0, name.getDomainLength(), getDomainLength()); This looks wrong. Look at the parameters in wildmatch: wildmatch(final String str, final String pat, int stri, final int strend, int pati, final int patend) What tests did you run to validate your changes? If none of the ObjectName tests fail with your changes you will need to add a new test that catches the error. - PR: https://git.openjdk.org/jdk/pull/11758
Re: RFR: 8299234: JMX Repository.query performance
On Wed, 21 Dec 2022 18:50:52 GMT, Alexey Bakhtin wrote: > Please find a patch to improve JMX Repository.query performance > > Using ObjectName.apply() allows significantly decrease memory usage and the > number of GC cycles: > Before: > > $ java test 100 100 > Test PASSED in 8943169791 ns. > GC: G1 Young Generation getCollectionCount()=177 getCollectionTime()=118 > > > After: > > $ java test 100 100 > Test PASSED in 4808213917 ns. > GC: G1 Young Generation getCollectionCount()=88 getCollectionTime()=53 > > Private ObjectName.matchDomains() method is also updated to minimize > unnecessary memory allocation. > > All javax/management jtreg tests passed successfully. I see several issues with the proposed changes: ObjectName is not final, so I'm not sure the changes proposed to ObjectName are safe when/if a subclass is supplied. And in addition, because it's not final, you will need to make a defensive copy of the input parameter in Repository::query and do the matching against the copy. - PR: https://git.openjdk.org/jdk/pull/11758
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v5]
On Fri, 16 Dec 2022 09:20:07 GMT, Daniel Fuchs wrote:
>> Damon Nguyen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert old translation. Fix lang codes
>
> src/java.base/share/classes/sun/launcher/resources/launcher_de.properties
> line 34:
>
>> 32: # Translators please note do not translate the options themselves
>> 33: java.launcher.opt.footer = \-cp > und ZIP-/JAR-Dateien>\n-classpath > und ZIP-/JAR-Dateien>\n--class-path > und ZIP-/JAR-Dateien>\n Eine durch {0} getrennte Liste mit
>> Verzeichnissen, JAR-Archiven\n und ZIP-Archiven, in denen
>> nach Klassendateien gesucht wird.\n-p \n--module-path
>> ...\n Eine durch {0} getrennte Liste mit
>> Verzeichnissen, von denen jedes Verzeichnis\n ein
>> Verzeichnis mit Modulen ist.\n--upgrade-module-path ...\n
>> Eine durch {0} getrennte Liste mit Verzeichnissen, von denen
>> jedes Verzeichnis\n ein Verzeichnis mit Modulen ist, die
>> upgradef\u00E4hige\n Module im Laufzeitimage ersetzen\n
>> --add-modules [,...]\n Root-Module,
>> die zus\u00E4tzlich zum anf\u00E4
nglichen Modul aufgel\u00F6st werden sollen.\n
kann auch wie folgt lauten: ALL-DEFAULT, ALL-SYSTEM,\n
ALL-MODULE-PATH.\n--enable-native-access [,...]\n
Module, die eingeschr\u00E4nkte native Vorg\u00E4nge
ausf\u00FChren d\u00FCrfen.\n kann auch
ALL-UNNAMED lauten.\n--list-modules\n Listet beobachtbare
Module auf und beendet den Vorgang\n-d \n--describe-module
\n Beschreibt ein Modul und beendet den Vorgang\n
--dry-run Erstellt eine VM und l\u00E4dt die Hauptklasse, f\u00FChrt aber
nicht die Hauptmethode aus.\n Die Option "--dry-run" kann
n\u00FCtzlich sein, um die\n Befehlszeilenoptionen, wie die
Modulsystemkonfiguration, zu validieren.\n--validate-modules\n
Validiert alle Module und beendet den Vorgang\n Die Option
"--val
idate-modules" kann n\u00FCtzlich sein, um\n Konflikte und
andere Fehler mit Modulen auf dem Modulpfad zu ermitteln.\n
-D=\n Legt eine Systemeigenschaft fest\n
-verbose:[class|module|gc|jni]\n Aktiviert die Verbose-Ausgabe
f\u00FCr das angegebene Subsystem\n-version Gibt die Produktversion an
den Fehlerstream aus und beendet den Vorgang\n--version Gibt die
Produktversion an den Outputstream aus und beendet den Vorgang\n
-showversion Gibt die Produktversion an den Fehlerstream aus und setzt den
Vorgang fort\n--show-version\n Gibt die Produktversion an
den Outputstream aus und setzt den Vorgang fort\n--show-module-resolution\n
Zeigt die Modulaufl\u00F6sungsausgabe beim Start an\n-? -h
-help\n Gibt diese Hilfemeldung an den Fehlerstream aus\n
--helpGibt diese Hilfemeldung an den Outputstream aus\n-X
Gibt Hilf
e zu zus\u00E4tzlichen Optionen an den Fehlerstream aus\n--help-extra
Gibt Hilfe zu zus\u00E4tzlichen Optionen an den Outputstream aus\n
-ea[:...|:]\n
-enableassertions[:...|:]\n Aktiviert
Assertions mit angegebener Granularit\u00E4t\n
-da[:...|:]\n
-disableassertions[:...|:]\n
Deaktiviert Assertions mit angegebener Granularit\u00E4t\n-esa |
-enablesystemassertions\n Aktiviert System-Assertions\n
-dsa | -disablesystemassertions\n Deaktiviert
System-Assertions\n-agentlib:[=]\n
L\u00E4dt die native Agent Library . Beispiel: -agentlib:jdwp\n
siehe auch -agentlib:jdwp=help\n
-agentpath:[=]\n L\u00E4dt die native Agent
Library mit dem vollst\u00E4ndigen Pfadnamen\n
-javaagent:[=]\
n \
>> 34: L\u00E4dt den Java-Programmiersprachen-Agent, siehe
>> java.lang.instrument\n-splash:\n Zeigt den
>> Startbildschirm mit einem angegebenen Bild an\n Skalierte
>> HiDPI-Bilder werden automatisch unterst\u00FCtzt und verwendet,\n
>> falls verf\u00FCgbar. Der nicht skalierte Bilddateiname (Beispiel:
>> image.ext)\n muss immer als Argument an die Option
>> "-splash" \u00FCbergeben werden.\n Das am besten geeignete
>> angegebene skalierte Bild wird\n automatisch
>> ausgew\u00E4hlt.\n Weitere Informationen finden Sie in der
>> Dokumentation zur SplashSc
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v5]
On Fri, 16 Dec 2022 03:38:54 GMT, Damon Nguyen wrote:
>> Open l10n drop
>> All tests passed
>
> Damon Nguyen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert old translation. Fix lang codes
src/java.base/share/classes/sun/launcher/resources/launcher_de.properties line
34:
> 32: # Translators please note do not translate the options themselves
> 33: java.launcher.opt.footer = \-cp und ZIP-/JAR-Dateien>\n-classpath ZIP-/JAR-Dateien>\n--class-path ZIP-/JAR-Dateien>\n Eine durch {0} getrennte Liste mit
> Verzeichnissen, JAR-Archiven\n und ZIP-Archiven, in denen
> nach Klassendateien gesucht wird.\n-p \n--module-path
> ...\n Eine durch {0} getrennte Liste mit
> Verzeichnissen, von denen jedes Verzeichnis\n ein
> Verzeichnis mit Modulen ist.\n--upgrade-module-path ...\n
> Eine durch {0} getrennte Liste mit Verzeichnissen, von denen
> jedes Verzeichnis\n ein Verzeichnis mit Modulen ist, die
> upgradef\u00E4hige\n Module im Laufzeitimage ersetzen\n
> --add-modules [,...]\n Root-Module,
> die zus\u00E4tzlich zum anf\u00E4n
glichen Modul aufgel\u00F6st werden sollen.\n
kann auch wie folgt lauten: ALL-DEFAULT, ALL-SYSTEM,\n
ALL-MODULE-PATH.\n--enable-native-access [,...]\n
Module, die eingeschr\u00E4nkte native Vorg\u00E4nge
ausf\u00FChren d\u00FCrfen.\n kann auch
ALL-UNNAMED lauten.\n--list-modules\n Listet beobachtbare
Module auf und beendet den Vorgang\n-d \n--describe-module
\n Beschreibt ein Modul und beendet den Vorgang\n
--dry-run Erstellt eine VM und l\u00E4dt die Hauptklasse, f\u00FChrt aber
nicht die Hauptmethode aus.\n Die Option "--dry-run" kann
n\u00FCtzlich sein, um die\n Befehlszeilenoptionen, wie die
Modulsystemkonfiguration, zu validieren.\n--validate-modules\n
Validiert alle Module und beendet den Vorgang\n Die Option
"--vali
date-modules" kann n\u00FCtzlich sein, um\n Konflikte und
andere Fehler mit Modulen auf dem Modulpfad zu ermitteln.\n
-D=\n Legt eine Systemeigenschaft fest\n
-verbose:[class|module|gc|jni]\n Aktiviert die Verbose-Ausgabe
f\u00FCr das angegebene Subsystem\n-version Gibt die Produktversion an
den Fehlerstream aus und beendet den Vorgang\n--version Gibt die
Produktversion an den Outputstream aus und beendet den Vorgang\n
-showversion Gibt die Produktversion an den Fehlerstream aus und setzt den
Vorgang fort\n--show-version\n Gibt die Produktversion an
den Outputstream aus und setzt den Vorgang fort\n--show-module-resolution\n
Zeigt die Modulaufl\u00F6sungsausgabe beim Start an\n-? -h
-help\n Gibt diese Hilfemeldung an den Fehlerstream aus\n
--helpGibt diese Hilfemeldung an den Outputstream aus\n-X
Gibt Hilfe
zu zus\u00E4tzlichen Optionen an den Fehlerstream aus\n--help-extra Gibt
Hilfe zu zus\u00E4tzlichen Optionen an den Outputstream aus\n
-ea[:...|:]\n
-enableassertions[:...|:]\n Aktiviert
Assertions mit angegebener Granularit\u00E4t\n
-da[:...|:]\n
-disableassertions[:...|:]\n
Deaktiviert Assertions mit angegebener Granularit\u00E4t\n-esa |
-enablesystemassertions\n Aktiviert System-Assertions\n
-dsa | -disablesystemassertions\n Deaktiviert
System-Assertions\n-agentlib:[=]\n
L\u00E4dt die native Agent Library . Beispiel: -agentlib:jdwp\n
siehe auch -agentlib:jdwp=help\n
-agentpath:[=]\n L\u00E4dt die native Agent
Library mit dem vollst\u00E4ndigen Pfadnamen\n
-javaagent:[=]\n
\
> 34: L\u00E4dt den Java-Programmiersprachen-Agent, siehe
> java.lang.instrument\n-splash:\n Zeigt den
> Startbildschirm mit einem angegebenen Bild an\n Skalierte
> HiDPI-Bilder werden automatisch unterst\u00FCtzt und verwendet,\n
> falls verf\u00FCgbar. Der nicht skalierte Bilddateiname (Beispiel:
> image.ext)\n muss immer als Argument an die Option "-splash"
> \u00FCbergeben werden.\n Das am besten geeignete angegebene
> skalierte Bild wird\n automatisch ausgew\u00E4hlt.\n
> Weitere Informationen finden Sie in der Dokumentation zur
> SplashScreen-API\n@argument files\n Eine oder mehrere
> Argumentdateien mit Optionen\n-disable-@files\n
> Verhindert die weitere Erweiterung von Argumentdateien\n
> --enable-preview\
Re: RFR: 8296546: Add @spec tags to API [v4]
On Thu, 1 Dec 2022 19:36:16 GMT, Jonathan Gibbons wrote: >> Please review a "somewhat automated" change to insert `@spec` tags into doc >> comments, as appropriate, to leverage the recent new javadoc feature to >> generate a new page listing the references to all external specifications >> listed in the `@spec` tags. >> >> "Somewhat automated" means that I wrote and used a temporary utility to scan >> doc comments looking for HTML links to selected sites, such as `ietf.org`, >> `unicode.org`, `w3.org`. These links may be in the main description of a doc >> comment, or in `@see` tags. For each link, the URL is examined, and >> "normalized", and inserted into the doc comment with a new `@spec` tag, >> giving the link and tile for the spec. >> >> "Normalized" means... >> * Use `https:` where possible (includes pretty much all cases) >> * Use a single consistent host name for all URLs coming from the same spec >> site (i.e. don't use different aliases for the same site) >> * Point to the root page of a multi-page spec >> * Use a consistent form of the spec, preferring HTML over plain text where >> both are available (this mostly applies to IETF specs) >> >> In addition, a "standard" title is determined for all specs, determined >> either from the content of the (main) spec page or from site index pages. >> >> The net effect is (or should be) that **all** the changes are to just >> **add** new `@spec` tags, based on the links found in each doc comment. >> There should be no other changes to the doc comments, or to the >> implementation of any classes and interfaces. >> >> That being said, the utility I wrote does have additional abilities, to >> update the links that it finds (e.g. changing to use `https:` etc,) but >> those features are _not_ being used here, but could be used in followup PRs >> if component teams so desired. I did notice while working on this overall >> feature that many of our links do point to "outdated" pages, some with >> eye-catching notices declaring that the spec has been superseded. >> Determining how, when and where to update such links is beyond the scope of >> this PR. >> >> Going forward, it is to be hoped that component teams will maintain the >> underlying links, and the URLs in `@spec` tags, such that if references to >> external specifications are updated, this will include updating the `@spec` >> tags. >> >> To see the effect of all these new `@spec` tags, see >> http://cr.openjdk.java.net/~jjg/8296546/api.00/ >> >> In particular, see the new [External >> Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) >> page, which you can also find via the new link near the top of the >> [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) >> pages. > > Jonathan Gibbons has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - Change ietf.org URLs to use rfc-editor.org > - Merge remote-tracking branch 'upstream/master' into 8296546.add-spec-tags > - Remove updates from unexported files > - Prefix RFC titles with `RFC :` > - JDK-8296547: Add @spec tags to API I have reviewed again the java.net, java.net.http, jdk.httpserver, java.naming, and javax.management changes - and I spotted a few places where the `@spec` duplicates an `@see` (noted below). I believe the duplicated `@see` should be removed now. src/java.base/share/classes/java/net/StandardSocketOptions.java line 62: > 60: * @spec https://www.rfc-editor.org/info/rfc919 RFC 919: Broadcasting > Internet Datagrams > 61: * @see http://www.ietf.org/rfc/rfc919.txt";>RFC 929: > 62: * Broadcasting Internet Datagrams This `@see` line should now be removed since it's referencing the exact same document. src/java.base/share/classes/java/net/StandardSocketOptions.java line 83: > 81: * @spec https://www.rfc-editor.org/info/rfc1122 RFC 1122: > Requirements for Internet Hosts - Communication Layers > 82: * @see http://www.ietf.org/rfc/rfc1122.txt";>RFC 1122 > 83: * Requirements for Internet Hosts -- Communication Layers Same remark here: please remove the `@see` src/java.base/share/classes/java/net/StandardSocketOptions.java line 154: > 152: * @spec https://www.rfc-editor.org/info/rfc1323 RFC 1323: TCP > Extensions for High Performance > 153: * @see http://www.ietf.org/rfc/rfc1323.txt";>RFC 1323: > TCP > 154: * Extensions for High Performance Remove the `@see` src/java.base/share/classes/java/net/StandardSocketOptions.java line 186: > 184: * > 185: * @spec https://www.rfc-editor.org/info/rfc793 RFC 793: > Transmission Control Protocol > 186: * @see http://www.ietf.org/rfc/rfc793.txt";>RFC 793: > Transmission Remove the @see src/java.base/share/classes/java/net/
Re: RFR: 8297794: Deprecate JMX Management Applets for Removal [v4]
On Wed, 30 Nov 2022 20:37:38 GMT, Kevin Walls wrote: >> Deprecate the Java Management Extension (JMX) Management Applet (m-let) >> feature for removal. >> >> This deprecation will have no impact on users of other JMX features, the >> JDK's built-in instrumentation, or any of the observability tools. >> >> More details in bug, and CSR JDK-8297795 > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > unnecessary Suppression nit LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11430
Re: RFR: 8297794: Deprecate JMX Management Applets for Removal [v3]
On Wed, 30 Nov 2022 17:02:45 GMT, Kevin Walls wrote:
>> Deprecate the Java Management Extension (JMX) Management Applet (m-let)
>> feature for removal.
>>
>> This deprecation will have no impact on users of other JMX features, the
>> JDK's built-in instrumentation, or any of the observability tools.
>>
>> More details in bug, and CSR JDK-8297795
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Deprecated annotation on the package-private classes
src/java.management/share/classes/javax/management/loading/MLetParser.java line
254:
> 252: * Parse the document pointed by the URL urlname
> 253: */
> 254: @SuppressWarnings({"deprecation", "removal"})
You might have to keep "removal", but I suspect "deprecation" should no longer
be necessary, now that this class is deprecated (though I might be wrong).
-
PR: https://git.openjdk.org/jdk/pull/11430
Re: RFR: 8297794: Deprecate JMX Management Applets for Removal
On Wed, 30 Nov 2022 14:24:36 GMT, Kevin Walls wrote: > MLetObjectInputStream and MLetParser are not public classes, so thinking they > are not part of the public API we need to deprecate before removal. Whether a class is public exported or not has no real relationship with whether it should have the `@Deprecated` annotation or not. It is better to add the `@Deprecated` annotation eagerly to all classes that are part of the feature being deprecated, if they are only used for the implementation of that feature, and if the thinking is that they would be removed if the feature is removed. Plus it usually simplifies things as it usually minimizes the places where you need to suppress warnings. - PR: https://git.openjdk.org/jdk/pull/11430
Re: RFR: 8297794: Deprecate JMX Management Applets for Removal
On Wed, 30 Nov 2022 12:08:22 GMT, Kevin Walls wrote:
> Deprecate the Java Management Extension (JMX) Management Applet (m-let)
> feature for removal.
>
> This deprecation will have no impact on users of other JMX features, the
> JDK's built-in instrumentation, or any of the observability tools.
>
> More details in bug, and CSR JDK-8297795
I have the same remark as Alan - I believe an `@deprecated ` text is needed in
the API documentation of the public exported classes that are deprecated. At
the minimum something like:
* @deprecated This class is deprecated for removal. There is no replacement.
I also see that you have chosen to add `@SuppressWarnings` in tests. Not sure
what the rules are for the serviceability area - but usually it's fine to keep
the deprecation warning in tests (that is: suppressing deprecation warnings in
tests is usually optional).
src/java.management/share/classes/javax/management/loading/MLetObjectInputStream.java
line 43:
> 41: class MLetObjectInputStream extends ObjectInputStream {
> 42:
> 43: @SuppressWarnings("removal")
Shouldn't `MLetObjectInputStream` be deprecated for removal too? I mean - if
MLet was removed - would we need to keep that class? If it were deprecated for
removal too then I suspect that there would be no need to suppress the warning
here (and below).
src/java.management/share/classes/javax/management/loading/MLetParser.java line
156:
> 154: * Scan an html file for {@literal } tags.
> 155: */
> 156: @SuppressWarnings("removal")
Same remark here. This class should probably be deprecated for removal too.
-
PR: https://git.openjdk.org/jdk/pull/11430
Re: RFR: 8296546: Add @spec tags to API [v3]
On Wed, 23 Nov 2022 18:57:03 GMT, Jonathan Gibbons wrote:
>> Please review a "somewhat automated" change to insert `@spec` tags into doc
>> comments, as appropriate, to leverage the recent new javadoc feature to
>> generate a new page listing the references to all external specifications
>> listed in the `@spec` tags.
>>
>> "Somewhat automated" means that I wrote and used a temporary utility to scan
>> doc comments looking for HTML links to selected sites, such as `ietf.org`,
>> `unicode.org`, `w3.org`. These links may be in the main description of a doc
>> comment, or in `@see` tags. For each link, the URL is examined, and
>> "normalized", and inserted into the doc comment with a new `@spec` tag,
>> giving the link and tile for the spec.
>>
>> "Normalized" means...
>> * Use `https:` where possible (includes pretty much all cases)
>> * Use a single consistent host name for all URLs coming from the same spec
>> site (i.e. don't use different aliases for the same site)
>> * Point to the root page of a multi-page spec
>> * Use a consistent form of the spec, preferring HTML over plain text where
>> both are available (this mostly applies to IETF specs)
>>
>> In addition, a "standard" title is determined for all specs, determined
>> either from the content of the (main) spec page or from site index pages.
>>
>> The net effect is (or should be) that **all** the changes are to just
>> **add** new `@spec` tags, based on the links found in each doc comment.
>> There should be no other changes to the doc comments, or to the
>> implementation of any classes and interfaces.
>>
>> That being said, the utility I wrote does have additional abilities, to
>> update the links that it finds (e.g. changing to use `https:` etc,) but
>> those features are _not_ being used here, but could be used in followup PRs
>> if component teams so desired. I did notice while working on this overall
>> feature that many of our links do point to "outdated" pages, some with
>> eye-catching notices declaring that the spec has been superseded.
>> Determining how, when and where to update such links is beyond the scope of
>> this PR.
>>
>> Going forward, it is to be hoped that component teams will maintain the
>> underlying links, and the URLs in `@spec` tags, such that if references to
>> external specifications are updated, this will include updating the `@spec`
>> tags.
>>
>> To see the effect of all these new `@spec` tags, see
>> http://cr.openjdk.java.net/~jjg/8296546/api.00/
>>
>> In particular, see the new [External
>> Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html)
>> page, which you can also find via the new link near the top of the
>> [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html)
>> pages.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove updates from unexported files
The java.base/net/, java.http/, java.naming/ changes look reasonable to me -
though like Alan I wonder if it wouldn't be better to have an inline `{@spec }`
tag - similar to `{@systemProperty }`, rather than repeating all the references
outside of the context where they were cited. This probably also calls for a
review of these references by maintainers of the various areas - as some of
them might need some updating - e.g. linking to `rfceditor` as was previously
suggested, and double checking whether all of them still make sense. Not
something to be conducted within this PR though.
-
PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: 8297192: Warning generating API docs for javax.management.MBeanServer: overridden methods do not document exception type
On Wed, 23 Nov 2022 12:31:26 GMT, Kevin Walls wrote: > Simple change to remove these mentions of RuntimeOperationsException. > > More notes in the bug, but this seems like a long-standing oversight that has > become the only warning in "make docs". Seems reasonable to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11318
Re: RFR: 8296546: Add @spec tags to API [v2]
On Tue, 22 Nov 2022 22:04:57 GMT, Jonathan Gibbons wrote: >> Please review a "somewhat automated" change to insert `@spec` tags into doc >> comments, as appropriate, to leverage the recent new javadoc feature to >> generate a new page listing the references to all external specifications >> listed in the `@spec` tags. >> >> "Somewhat automated" means that I wrote and used a temporary utility to scan >> doc comments looking for HTML links to selected sites, such as `ietf.org`, >> `unicode.org`, `w3.org`. These links may be in the main description of a doc >> comment, or in `@see` tags. For each link, the URL is examined, and >> "normalized", and inserted into the doc comment with a new `@spec` tag, >> giving the link and tile for the spec. >> >> "Normalized" means... >> * Use `https:` where possible (includes pretty much all cases) >> * Use a single consistent host name for all URLs coming from the same spec >> site (i.e. don't use different aliases for the same site) >> * Point to the root page of a multi-page spec >> * Use a consistent form of the spec, preferring HTML over plain text where >> both are available (this mostly applies to IETF specs) >> >> In addition, a "standard" title is determined for all specs, determined >> either from the content of the (main) spec page or from site index pages. >> >> The net effect is (or should be) that **all** the changes are to just >> **add** new `@spec` tags, based on the links found in each doc comment. >> There should be no other changes to the doc comments, or to the >> implementation of any classes and interfaces. >> >> That being said, the utility I wrote does have additional abilities, to >> update the links that it finds (e.g. changing to use `https:` etc,) but >> those features are _not_ being used here, but could be used in followup PRs >> if component teams so desired. I did notice while working on this overall >> feature that many of our links do point to "outdated" pages, some with >> eye-catching notices declaring that the spec has been superseded. >> Determining how, when and where to update such links is beyond the scope of >> this PR. >> >> Going forward, it is to be hoped that component teams will maintain the >> underlying links, and the URLs in `@spec` tags, such that if references to >> external specifications are updated, this will include updating the `@spec` >> tags. >> >> To see the effect of all these new `@spec` tags, see >> http://cr.openjdk.java.net/~jjg/8296546/api.00/ >> >> In particular, see the new [External >> Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) >> page, which you can also find via the new link near the top of the >> [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) >> pages. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Prefix RFC titles with `RFC :` Thanks for adding the RFC prefix to the RFC link. What is the purpose of editing non exported classes though, like those in the `sun.net` subpackages? - PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: JDK-8297164: Update troff man pages and CheckManPageOptions.java [v2]
On Mon, 21 Nov 2022 17:54:15 GMT, Jonathan Gibbons wrote: >> Please review an update for the troff man pages, following the recent update >> to upgrade to use pandoc 2.19.2 >> (See https://bugs.openjdk.org/browse/JDK-8297165) >> >> In conjunction with this, one javadoc test also needs to be updated, to work >> with the new form of output generated by the new version of pandoc. > > Jonathan Gibbons has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Fix merge issue > - Merge with upstream/master > - JDK-8297164: Update troff man pages and CheckManPageOptions.java The diffs are a bit difficult to read but I didn't spot anything obviously wrong with jwebserver. I could see that the new compress option was there in jmod man page too. +1. - PR: https://git.openjdk.org/jdk/pull/11223
Re: RFR: 8296953: Fix a typo in exception documentation
On Mon, 14 Nov 2022 19:04:49 GMT, Pavel Rappo wrote: > Please review this trivial documentation change that fixes a typo > accidentally found while comparing javadoc output for an unrelated change in > jdk.javadoc. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11140
Re: RFR: 8296546: Add @spec tags to API
On Fri, 11 Nov 2022 11:45:43 GMT, Lance Andersen wrote: > It would probably be easier for the reviewers and for you if the PR could be > broken out by areas into separate PRs Leaving out the non-public and non-exported classes would also reduce the PR size. - PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: 8296546: Add @spec tags to API
On Thu, 10 Nov 2022 21:56:26 GMT, Jonathan Gibbons wrote: > On the same text but linking to different RFCs: that's tantamount to a bug > somewhere. The spec for `@spec` dictates that the URLs and titles should be > in 1-1 correspondence, and this is supposed to be enforced in the docket. In > other words, specs should have unique titles, and any title should only be > used for one spec. It's not uncommon for a newer version of a RFC to change its number but keep its title. I see that the links in the class level API documentation both have the RFC number in their link text. Somehow that was stripped by your tool - possibly because it tried to extract some meta information from the linked page itself? - PR: https://git.openjdk.org/jdk/pull/11073
Re: RFR: JDK-8296547: Add @spec tags to API
On Thu, 10 Nov 2022 01:10:13 GMT, Jonathan Gibbons wrote: > Please review a "somewhat automated" change to insert `@spec` tags into doc > comments, as appropriate, to leverage the recent new javadoc feature to > generate a new page listing the references to all external specifications > listed in the `@spec` tags. > > "Somewhat automated" means that I wrote and used a temporary utility to scan > doc comments looking for HTML links to selected sites, such as `ietf.org`, > `unicode.org`, `w3.org`. These links may be in the main description of a doc > comment, or in `@see` tags. For each link, the URL is examined, and > "normalized", and inserted into the doc comment with a new `@spec` tag, > giving the link and tile for the spec. > > "Normalized" means... > * Use `https:` where possible (includes pretty much all cases) > * Use a single consistent host name for all URLs coming from the same spec > site (i.e. don't use different aliases for the same site) > * Point to the root page of a multi-page spec > * Use a consistent form of the spec, preferring HTML over plain text where > both are available (this mostly applies to IETF specs) > > In addition, a "standard" title is determined for all specs, determined > either from the content of the (main) spec page or from site index pages. > > The net effect is (or should be) that **all** the changes are to just **add** > new `@spec` tags, based on the links found in each doc comment. There should > be no other changes to the doc comments, or to the implementation of any > classes and interfaces. > > That being said, the utility I wrote does have additional abilities, to > update the links that it finds (e.g. changing to use `https:` etc,) but those > features are _not_ being used here, but could be used in followup PRs if > component teams so desired. I did notice while working on this overall > feature that many of our links do point to "outdated" pages, some with > eye-catching notices declaring that the spec has been superseded. Determining > how, when and where to update such links is beyond the scope of this PR. > > Going forward, it is to be hoped that component teams will maintain the > underlying links, and the URLs in `@spec` tags, such that if references to > external specifications are updated, this will include updating the `@spec` > tags. > > To see the effect of all these new `@spec` tags, see > http://cr.openjdk.java.net/~jjg/8296546/api.00/ > > In particular, see the new [External > Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) > page, which you can also find via the new link near the top of the > [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) > pages. Hi Jon, When referencing an RFC, it might be good to keep the RFC number in the text link. For instance I see that java.net.URL now has this: http://cr.openjdk.java.net/~jjg/8296546/api.00/java.base/java/net/URL.html External Specifications [Format for Literal IPv6 Addresses in URL's](https://www.ietf.org/rfc/rfc2732.html), [Uniform Resource Identifier (URI): Generic Syntax](https://www.ietf.org/rfc/rfc3986.html), [Uniform Resource Identifiers (URI): Generic Syntax](https://www.ietf.org/rfc/rfc2396.html) You will see that two of the RFC links have the same text but link to different RFCs, which I am finding confusing. Also I do hope it's clear that if a specification is referenced it doesn't mean it's being implemented. - PR: https://git.openjdk.org/jdk/pull/11073
Integrated: 8294241: Deprecate URL public constructors
On Wed, 26 Oct 2022 16:00:56 GMT, Daniel Fuchs wrote:
> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
> to parse or construct any URL.
>
> The `java.net.URL` class does not itself encode or decode any URL components
> according to the escaping mechanism defined in RFC2396. It is the
> responsibility of the caller to encode any fields, which need to be escaped
> prior to calling URL, and also to decode any escaped fields, that are
> returned from URL.
>
> This has lead to many issues in the past. Indeed, if used improperly, there
> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
> URL string that can be parsed back into the same URL. This can lead to
> constructing misleading URLs. Another issue is with `equals()` and
> `hashCode()` which may have to perform a lookup, and do not take
> encoding/escaping into account.
>
> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
> of the shortcoming of `java.net.URL`. Conversion methods to create a URL from
> a URI were also added. However, it was left up to the developers to use
> `java.net.URI`, or not. This RFE proposes to deprecate all public
> constructors of `java.net.URL`, in order to provide a stronger warning about
> their potential misuses. To construct a URL, using `URI::toURL` should be
> preferred.
>
> In order to provide an alternative to the constructors that take a stream
> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
> java.net.URLStreamHandler)` is provided as part of this change.
>
> Places in the JDK code base that were constructing `java.net.URL` have been
> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
> issues will be logged to revisit the calling code.
>
> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
This pull request has now been integrated.
Changeset: 4338f527
Author:Daniel Fuchs
URL:
https://git.openjdk.org/jdk/commit/4338f527aa81350e3636dcfbcd2eb17ddaad3914
Stats: 853 lines in 82 files changed: 707 ins; 5 del; 141 mod
8294241: Deprecate URL public constructors
Reviewed-by: joehw, prr, alanb, aefimov, michaelm
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v5]
> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
> to parse or construct any URL.
>
> The `java.net.URL` class does not itself encode or decode any URL components
> according to the escaping mechanism defined in RFC2396. It is the
> responsibility of the caller to encode any fields, which need to be escaped
> prior to calling URL, and also to decode any escaped fields, that are
> returned from URL.
>
> This has lead to many issues in the past. Indeed, if used improperly, there
> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
> URL string that can be parsed back into the same URL. This can lead to
> constructing misleading URLs. Another issue is with `equals()` and
> `hashCode()` which may have to perform a lookup, and do not take
> encoding/escaping into account.
>
> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
> of the shortcoming of `java.net.URL`. Conversion methods to create a URL from
> a URI were also added. However, it was left up to the developers to use
> `java.net.URI`, or not. This RFE proposes to deprecate all public
> constructors of `java.net.URL`, in order to provide a stronger warning about
> their potential misuses. To construct a URL, using `URI::toURL` should be
> preferred.
>
> In order to provide an alternative to the constructors that take a stream
> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
> java.net.URLStreamHandler)` is provided as part of this change.
>
> Places in the JDK code base that were constructing `java.net.URL` have been
> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
> issues will be logged to revisit the calling code.
>
> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
Daniel Fuchs has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains 10 additional commits since the
last revision:
- Merge branch 'master' into deprecate-url-ctor-8294241
- Update src/java.base/share/classes/java/net/URL.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Update src/java.base/share/classes/java/net/URL.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Update src/java.base/share/classes/java/net/URL.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Integrated review feedback
- Merge branch 'master' into deprecate-url-ctor-8294241
- Updated after review comments. In particular var tmp => var => _unused - and
avoid var in java.xml
- Merge branch 'master' into deprecate-url-ctor-8294241
- Fix whitespace issues
- 8294241
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10874/files
- new: https://git.openjdk.org/jdk/pull/10874/files/fc899005..b4a73f40
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=03-04
Stats: 42853 lines in 291 files changed: 10793 ins; 30812 del; 1248 mod
Patch: https://git.openjdk.org/jdk/pull/10874.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10874/head:pull/10874
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v3]
On Thu, 3 Nov 2022 07:42:44 GMT, ExE Boss wrote:
>> Daniel Fuchs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains six additional
>> commits since the last revision:
>>
>> - Integrated review feedback
>> - Merge branch 'master' into deprecate-url-ctor-8294241
>> - Updated after review comments. In particular var tmp => var => _unused -
>> and avoid var in java.xml
>> - Merge branch 'master' into deprecate-url-ctor-8294241
>> - Fix whitespace issues
>> - 8294241
>
> src/java.base/share/classes/java/net/URL.java line 885:
>
>> 883: @SuppressWarnings("deprecation")
>> 884: var result = new URL("jrt", host, port, file, null);
>> 885: return result;
>
> Suggestion:
>
> return new URL("jrt", host, port, file, null);
Ah ! Good point thanks! I have accepted your suggestions.
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v4]
On Thu, 3 Nov 2022 10:56:28 GMT, Michael McMahon wrote: >> Daniel Fuchs has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - Update src/java.base/share/classes/java/net/URL.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Update src/java.base/share/classes/java/net/URL.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> - Update src/java.base/share/classes/java/net/URL.java >> >>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > > src/java.base/share/classes/java/net/URL.java line 152: > >> 150: * provide any guarantee about its conformance to the URL >> 151: * syntax specification. >> 152: * > > I wonder do we need a stronger statement about potential incompatibility here > (line 149 - 151)? > > Something to the effect that - due to URI's stricter parsing rules not all > existing URL instances can be represented as URI's and some that are may turn > out to be opaque unexpectedly instead of hierarchical. That would better go in URL::toURI I think - but AFAICS would only apply if you constructed the URL from one of the deprecated constructors. It's not a bad idea - but given that the CSR has been approved I'd rather track that in a follow up PR. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v4]
> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
> to parse or construct any URL.
>
> The `java.net.URL` class does not itself encode or decode any URL components
> according to the escaping mechanism defined in RFC2396. It is the
> responsibility of the caller to encode any fields, which need to be escaped
> prior to calling URL, and also to decode any escaped fields, that are
> returned from URL.
>
> This has lead to many issues in the past. Indeed, if used improperly, there
> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
> URL string that can be parsed back into the same URL. This can lead to
> constructing misleading URLs. Another issue is with `equals()` and
> `hashCode()` which may have to perform a lookup, and do not take
> encoding/escaping into account.
>
> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
> of the shortcoming of `java.net.URL`. Conversion methods to create a URL from
> a URI were also added. However, it was left up to the developers to use
> `java.net.URI`, or not. This RFE proposes to deprecate all public
> constructors of `java.net.URL`, in order to provide a stronger warning about
> their potential misuses. To construct a URL, using `URI::toURL` should be
> preferred.
>
> In order to provide an alternative to the constructors that take a stream
> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
> java.net.URLStreamHandler)` is provided as part of this change.
>
> Places in the JDK code base that were constructing `java.net.URL` have been
> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
> issues will be logged to revisit the calling code.
>
> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
Daniel Fuchs has updated the pull request incrementally with three additional
commits since the last revision:
- Update src/java.base/share/classes/java/net/URL.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Update src/java.base/share/classes/java/net/URL.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Update src/java.base/share/classes/java/net/URL.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10874/files
- new: https://git.openjdk.org/jdk/pull/10874/files/f6b8a9f9..fc899005
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=02-03
Stats: 8 lines in 1 file changed: 0 ins; 5 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/10874.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10874/head:pull/10874
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Tue, 1 Nov 2022 14:47:49 GMT, Alan Bateman wrote: >> Actually... Maybe I could move up the paragraph that says that URL >> constructors are deprecated up here, just after thetitle? Would >> that be better? > > Try it, it might be better. I think the main thing is that link brings the > reader to somewhere close to the deprecated message. Done. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v3]
> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
> to parse or construct any URL.
>
> The `java.net.URL` class does not itself encode or decode any URL components
> according to the escaping mechanism defined in RFC2396. It is the
> responsibility of the caller to encode any fields, which need to be escaped
> prior to calling URL, and also to decode any escaped fields, that are
> returned from URL.
>
> This has lead to many issues in the past. Indeed, if used improperly, there
> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
> URL string that can be parsed back into the same URL. This can lead to
> constructing misleading URLs. Another issue is with `equals()` and
> `hashCode()` which may have to perform a lookup, and do not take
> encoding/escaping into account.
>
> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
> of the shortcoming of `java.net.URL`. Conversion methods to create a URL from
> a URI were also added. However, it was left up to the developers to use
> `java.net.URI`, or not. This RFE proposes to deprecate all public
> constructors of `java.net.URL`, in order to provide a stronger warning about
> their potential misuses. To construct a URL, using `URI::toURL` should be
> preferred.
>
> In order to provide an alternative to the constructors that take a stream
> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
> java.net.URLStreamHandler)` is provided as part of this change.
>
> Places in the JDK code base that were constructing `java.net.URL` have been
> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
> issues will be logged to revisit the calling code.
>
> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
Daniel Fuchs has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase. The pull request contains six additional commits since the
last revision:
- Integrated review feedback
- Merge branch 'master' into deprecate-url-ctor-8294241
- Updated after review comments. In particular var tmp => var => _unused - and
avoid var in java.xml
- Merge branch 'master' into deprecate-url-ctor-8294241
- Fix whitespace issues
- 8294241
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10874/files
- new: https://git.openjdk.org/jdk/pull/10874/files/fd4ca287..f6b8a9f9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=01-02
Stats: 3893 lines in 203 files changed: 2469 ins; 611 del; 813 mod
Patch: https://git.openjdk.org/jdk/pull/10874.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10874/head:pull/10874
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Tue, 1 Nov 2022 14:10:01 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/java/net/URL.java line 133:
>>
>>> 131: * specified. The optional fragment is not inherited.
>>> 132: *
>>> 133: * Constructing instances of
>>> {@code URL}
>>
>> Would it be better to move the anchor to line 164 (the line where it says
>> that the URL constructors are deprecated?
>
> To be discussed: I actually wanted the deprecation link ( the link from
> `@deprecated` ) to lead here because I find that the whole section is
> relevant for developers who might want to decide whether to actually move
> away from using constructors, or be tempted to just use `@SuppressWarnings`.
Actually... Maybe I could move up the paragraph that says that URL constructors
are deprecated up here, just after the <h2> title? Would that be better?
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Sat, 29 Oct 2022 14:24:09 GMT, Alan Bateman wrote:
>> Daniel Fuchs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - Updated after review comments. In particular var tmp => var => _unused -
>> and avoid var in java.xml
>> - Merge branch 'master' into deprecate-url-ctor-8294241
>> - Fix whitespace issues
>> - 8294241
>
> src/java.base/share/classes/java/net/URL.java line 133:
>
>> 131: * specified. The optional fragment is not inherited.
>> 132: *
>> 133: * Constructing instances of
>> {@code URL}
>
> Would it be better to move the anchor to line 164 (the line where it says
> that the URL constructors are deprecated?
To be discussed: I actually wanted the deprecation link ( the link from
`@deprecated` ) to lead here because I find that the whole section is relevant
for developers who might want to decide whether to actually move away from
using constructors, or be tempted to just use `@SuppressWarnings`.
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Sat, 29 Oct 2022 14:17:12 GMT, Alan Bateman wrote:
>> Daniel Fuchs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - Updated after review comments. In particular var tmp => var => _unused -
>> and avoid var in java.xml
>> - Merge branch 'master' into deprecate-url-ctor-8294241
>> - Fix whitespace issues
>> - 8294241
>
> src/java.base/share/classes/java/net/URL.java line 166:
>
>> 164: * The {@code java.net.URL} constructors are deprecated.
>> 165: * Developers are encouraged to use {@link URI java.net.URI} to parse
>> 166: * or construct any {@code URL}. In cases where an instance of {@code
>
> "any URL" -> "a URL" or "all URLs".
done
> src/java.base/share/classes/java/net/URL.java line 168:
>
>> 166: * or construct any {@code URL}. In cases where an instance of {@code
>> 167: * java.net.URL} is needed to open a connection, {@link URI} can be used
>> 168: * to construct or parse the URL string, possibly calling {@link
>
> I wonder if it might be clearer to say "url string", only to avoid anyone
> thinking they call URL::toString.
I don't believe it would be syntactically correct to put it in all lower case
since URL is an acronym. I could replace it with "URI string" instead but I'm
not sure it would be better. What do you think?
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Sat, 29 Oct 2022 14:16:24 GMT, Alan Bateman wrote:
>> Daniel Fuchs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - Updated after review comments. In particular var tmp => var => _unused -
>> and avoid var in java.xml
>> - Merge branch 'master' into deprecate-url-ctor-8294241
>> - Fix whitespace issues
>> - 8294241
>
> src/java.base/share/classes/java/net/URL.java line 157:
>
>> 155: * The URL constructors are specified to throw
>> 156: * {@link MalformedURLException} but the actual parsing/validation
>> 157: * that are performed is implementation dependent. Some
>> parsing/validation
>
> "the ... are performed" -> "the ... is performed".
done
> src/java.base/share/classes/java/net/URL.java line 852:
>
>> 850: * @since 20
>> 851: */
>> 852: public static URL of(URI uri, URLStreamHandler streamHandler)
>
> The parameter is named "handler" rather than "streamHandler" in constructors
> so we should probably keep it the same to avoid any confusion.
done
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Sat, 29 Oct 2022 14:14:22 GMT, Alan Bateman wrote:
>> Daniel Fuchs has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - Updated after review comments. In particular var tmp => var => _unused -
>> and avoid var in java.xml
>> - Merge branch 'master' into deprecate-url-ctor-8294241
>> - Fix whitespace issues
>> - 8294241
>
> src/java.base/share/classes/java/net/URL.java line 885:
>
>> 883:
>> 884: @SuppressWarnings("deprecation")
>> 885: var result = new URL("jrt", host, port, file, null);
>
> The URL scheme for jrt does not have a port so we should look at that some
> time.
Noted.
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Mon, 31 Oct 2022 22:00:01 GMT, Phil Race wrote: > Deprecate URL constructors. Developers are encouraged to use java.net.URI to > parse or construct any URL. ... To construct a URL, using URI::toURL should > be preferred. > > You have jumped through some refactoring hoops to be able to apply the > deprecation suppression to as little code as possible .. having made such > changes, then why didn't you just make the recommended change instead ? > > Should I presume that the recommended route will have some nasty little > incompatibilities we will need to be careful of first ? Possibly yes. Using URI where it was not used before might cause some behavioral changes, that would need to be examined and possibly documented through separate CSRs. This is better handled on a case-by-case basis for each area affected. The changes as currently proposed will not lead to any behavioral difference. > > And what about Peter Firmstone's comment "We stopped using java.net.URI some > years ago as it's obsolete.?" > > I can't reconcile that with the recommendation to use it .. URI implements RFC 2396 with some deviations, noted in its API documentation, which make it a crossbreed between RFC 2396 and RFC 3986. As Alan noted earlier, changing URI to strictly implement RFC 3986 is not a compatible move: it was attempted in JDK 6 but had to backed out quickly as it caused widespread breakage. But even if it doesn't implement the latest RFC strictly, it's still a much more modern API and implementation than java.net.URL. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8295729: Add jcheck whitespace checking for properties files [v3]
On Mon, 24 Oct 2022 19:21:07 GMT, Magnus Ihse Bursie wrote: >> Properties files is essentially source code. It should have the same >> whitespace checks as all other source code, so we don't get spurious >> trailing whitespace changes. >> >> With the new Skara jcheck, it is possible to increase the coverage of the >> whitespace checks (in the old mercurial version, this was more or less >> impossible). >> >> The only manual change is to `.jcheck/conf`. All other changes were made by >> running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ >> \t]*$//'`. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Revert "Remove check for .properties from jcheck" > >This reverts commit c91fdaa19dc06351598bd1c0614e1af3bfa08ae2. > - Change trailing space and tab in values to unicode encoding Changes to java (resp sun) .util.logging tests and javax.management tests look good to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10792
