Re: RFR: 8331670: Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal [v3]
On Tue, 21 May 2024 07:26:17 GMT, Alan Bateman wrote: >> This is the implementation changes for JEP 471. >> >> The methods in sun.misc.Unsafe for on-heap and off-heap access are >> deprecated for removal. This means a removal warning at compile time. No >> methods have been removed. A deprecated message is added to each of the >> methods but unlikely to be seen as the JDK does not generate or publish the >> API docs for this class. >> >> A new command line option --sun-misc-unsafe-memory-access=$value is >> introduced to allow or deny access to these methods. The default proposed >> for JDK 23 is "allow" so no change in behavior compared to JDK 22 or >> previous releases. >> >> A new test is added to test the command line option settings. The existing >> micros for FFM that use Unsafe are updated to suppress the removal warning >> at compile time. A new micro is introduced with a small sample of methods to >> ensure the changes doesn't cause any perf regressions ([sample >> results](https://cr.openjdk.org/~alanb/8331670-results.txt)). >> >> For now, the changes include the update to the man page for the "java" >> command. It might be that this has to be separated out so that it goes with >> other updates in the release. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge > - Add removal to DISABLED_WARNINGS >Fail at startup if bad value specified to option > - Merge > - Update man page > - Update man page > - Fix comment > - Merge > - Merge > - Whitespace > - Initial commit src/jdk.unsupported/share/classes/sun/misc/Unsafe.java line 1750: > 1748: } > 1749: > 1750: // Instructure for --sun-misc-unsafe-memory-access= command > line option. Suggestion: // Infrastructure for --sun-misc-unsafe-memory-access= command line option. - PR Review Comment: https://git.openjdk.org/jdk/pull/19174#discussion_r1613644783
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
On Tue, 28 May 2024 22:31:15 GMT, Jonathan Gibbons wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > incorporate review comments I like `//---` ; +1! - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2137452920
RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that loggers are GC'ed (or not GC'ed) after a reset or an update. For that they poll a ReferenceQueue in a loop. The number of iteration is adjusted according to the jtreg timeout factor. However, the logic in the test did not expect that the timeout might be less than 1. This fix does two things: 1. fix the adjustCount logic - so that the number of iteration can only be increased 2. use remove(timeout) instead of poll() in order to minimize the waiting time. - Commit messages: - 8333270 Changes: https://git.openjdk.org/jdk/pull/19503/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19503&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8333270 Stats: 11 lines in 2 files changed: 0 ins; 2 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/19503.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19503/head:pull/19503 PR: https://git.openjdk.org/jdk/pull/19503
Re: RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 [v2]
> HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that > loggers are GC'ed (or not GC'ed) after a reset or an update. For that they > poll a ReferenceQueue in a loop. The number of iteration is adjusted > according to the jtreg timeout factor. However, the logic in the test did not > expect that the timeout might be less than 1. > > This fix does two things: > > 1. fix the adjustCount logic - so that the number of iteration can only be > increased > 2. use remove(timeout) instead of poll() in order to minimize the waiting > time. Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Review feedback: use Reference::refersTo consistently - Changes: - all: https://git.openjdk.org/jdk/pull/19503/files - new: https://git.openjdk.org/jdk/pull/19503/files/be5a4d82..75576a8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19503&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19503&range=00-01 Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/19503.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19503/head:pull/19503 PR: https://git.openjdk.org/jdk/pull/19503
Re: RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 [v2]
On Sat, 1 Jun 2024 05:20:24 GMT, Jaikiran Pai wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review feedback: use Reference::refersTo consistently
>
> test/jdk/java/util/logging/LogManager/Configuration/updateConfiguration/HandlersOnComplexResetUpdate.java
> line 219:
>
>> 217: }
>> 218: WeakReference barRef = new
>> WeakReference<>(Logger.getLogger("com.bar"), queue);
>> 219: if (!barRef.refersTo(barChild.getParent())) {
>
> Hello Daniel, since `refersTo()` is the preferred API in cases like this,
> should this same change be done in the other `HandlersOnComplexUpdate` test
> that's being updated in this PR?
Good point. Updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19503#discussion_r1626196539
Integrated: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
On Fri, 31 May 2024 14:55:57 GMT, Daniel Fuchs wrote: > HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests verify that > loggers are GC'ed (or not GC'ed) after a reset or an update. For that they > poll a ReferenceQueue in a loop. The number of iteration is adjusted > according to the jtreg timeout factor. However, the logic in the test did not > expect that the timeout might be less than 1. > > This fix does two things: > > 1. fix the adjustCount logic - so that the number of iteration can only be > increased > 2. use remove(timeout) instead of poll() in order to minimize the waiting > time. This pull request has now been integrated. Changeset: d02cb742 Author:Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/d02cb742f79e88c6438ca58a6357fe432fb286cb Stats: 16 lines in 2 files changed: 0 ins; 2 del; 14 mod 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/19503
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: 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 [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 [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 [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 [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: 8334490: Normalize string with locale invariant `toLowerCase()`
On Tue, 18 Jun 2024 18:23:12 GMT, Naoto Sato wrote: > The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for > string comparison, which should be avoided. +1 - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19775#pullrequestreview-2126251008
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: [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: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]
On Wed, 7 Aug 2024 15:59:14 GMT, Brian Burkhalter wrote:
>> This proposed change would move the native objects required for NIO file
>> interaction from the libnio native library to the libjava native library on
>> Linux, macOS, and Windows.
>
> Brian Burkhalter 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:
>
> - Merge
> - 8337143: Removed dependency of libjava on headers in libnio/ch
> - 8337143: Move natives to /native/libjava/nio/{ch,fs} as a
> function of their original location in libnio
> - 8337143: (fc, fs) Move filesystem-related native objects from libnio to
> libjava
Disclaimer: I have not tried to understand the proposed change in details.
However I have spotted a small oddity.
src/java.base/share/classes/java/net/Inet6AddressImpl.java line 154:
> 152: static {
> 153: jdk.internal.loader.BootLoader.loadLibrary("net");
> 154: }
I am curious about this change - wouldn't it be needed in Inet4AddressImpl as
well?
src/java.base/windows/native/libjava/IOUtil.c line 37:
> 35: #include "nio.h"
> 36: #include "nio_util.h"
> 37: /* #include "net_util.h" */
Is this change intended or is this a left over from some experimentation?
-
PR Review: https://git.openjdk.org/jdk/pull/20317#pullrequestreview-2227299000
PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709024031
PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709029492
Re: RFR: 8336926: jdk/internal/util/ReferencedKeyTest.java can fail with ConcurrentModificationException
On Wed, 7 Aug 2024 19:26:59 GMT, Roger Riggs wrote: > The original test fails intermittently, the reproducer failed consistently. > With the fix, the failure was not observed in the test or reproducer. > > In jdk.internal.util.ReferencedKeyMap.entrySet() and toString() methods, > avoid removing stale references that would modify the keyset while it is > being iterated over. > If GC removes the key, iterating or streaming over the keyset might get a > ConcurrentModificationException. The proposed changes look reasonable to me. Those seem to be the only methods where removeStaleReferences() could have been indirectly called while looping over the entries. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20499#pullrequestreview-2227972196
Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]
On Thu, 8 Aug 2024 14:32:25 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/net/Inet6AddressImpl.java line 154:
>>
>>> 152: static {
>>> 153: jdk.internal.loader.BootLoader.loadLibrary("net");
>>> 154: }
>>
>> I am curious about this change - wouldn't it be needed in Inet4AddressImpl
>> as well?
>
> I have not seen any failures in CI testing. Is there a specific test that
> would reveal whether this is a problem?
It may be because we have no IPv4 only machine in the CI? It seems strange that
IPv6 is treated differently than IPv4 in that respect.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709742606
Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]
On Thu, 8 Aug 2024 16:09:55 GMT, Brian Burkhalter wrote: >> It may be because we have no IPv4 only machine in the CI? It seems strange >> that IPv6 is treated differently than IPv4 in that respect. > > How would you suggest testing this? I don't know - you added that code to Inet6AddressImpl - so presumably a test was failing without that code? Which test was that? It wasn't obvious to me that adding code to load the "net" library would be required here. - PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709861717
Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]
On Thu, 8 Aug 2024 16:21:30 GMT, Brian Burkhalter wrote: >> I don't know - you added that code to Inet6AddressImpl - so presumably a >> test was failing without that code? >> Which test was that? It wasn't obvious to me that adding code to load the >> "net" library would be required here. > > I'll have to investigate. Possibly - if you made isIPv6Supported() in InetAddress.c return false, you might be able to see the issue in the same test that you observed failing without your change. InetAddress has a static block that will load the "net" library, and an other static block that will create the InetAddressImpl. It's a bit curious because I would expect the block that loads the "net" library to be executed before. And the only place where I see Inet6AddressImpl being used is in that second static block in InetAddress. I am not an expert on library loading though :-( - PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1709897049
Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]
On Thu, 8 Aug 2024 21:23:58 GMT, Brian Burkhalter wrote: >> Thanks for the suggestions. I will look into it. > > Without loading libnet in Inet6AddressImpl, the test > java/net/InetAddress/NullCharInHostnameDriver.java fails with > UnsatisfiedLinkError: > > java.lang.UnsatisfiedLinkError: 'java.net.InetAddress[] > java.net.Inet6AddressImpl.lookupAllHostAddr(java.lang.String, int)' > at java.base/java.net.Inet6AddressImpl.lookupAllHostAddr(Native Method) > at > java.base/java.net.Inet6AddressImpl.lookupAllHostAddr(Inet6AddressImpl.java:52) > at > java.base/java.net.NullCharInHostname.main(NullCharInHostname.java:37) > > This is on Unix (Linux, macOS) only, not on Windows. OK, this test uses a private API to create an instance of Inet6AddressImpl; This explain why in this test Inet6AddressImpl gets loaded before InetAddress. var impl = new Inet6AddressImpl(); It should never happen outside of this test. Now the tricky question: what in your proposed changes caused "net" not to be loaded before the test created new Inet6AddressImpl(); ? - PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1711100272
Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v4]
On Fri, 9 Aug 2024 15:09:08 GMT, Brian Burkhalter wrote:
>> OK, this test uses a private API to create an instance of Inet6AddressImpl;
>> This explain why in this test Inet6AddressImpl gets loaded before
>> InetAddress.
>>
>> var impl = new Inet6AddressImpl();
>>
>> It should never happen outside of this test. Now the tricky question: what
>> in your proposed changes caused "net" not to be loaded before the test
>> created new Inet6AddressImpl(); ?
>
> Loading "net" was removed from IOUtil so I am thinking that IOUtil must have
> been initialized somewhere before constructing Inet6AddressImpl, but I've not
> identified where just yet.
I see. Inet6AddressImpl and Inet4AddressImpl are symetric classes implementing
InetAddressImpl. Both will make native calls to the "net" library - so both
require the library to be loaded.
In the JDK, these two classes are only loaded by InetAddress, after the "net"
library has been loaded.
The test test java/net/InetAddress/NullCharInHostnameDriver.java seems to be an
outlier (@AlekseiEfimov ?) which for some reason uses a private API (the test
is injected in java.base) and create a new instance of Inet6AddressImpl before
InetAddress is loaded. This is why without this change to Inet6AddressImpl we
get the UnsatisfiedLinkError. We would have gotten the same from
Inet4AddressImpl if the test had first required access to Inet4AddressImpl
instead.
So it seems that we should either add the call to load the "net" library to
both Inet6AddressImpl and Inet4AddressImpl for symetry (there doesn't seem to
be any reason why Inet6AddressImpl would be preferred in this respect), or
remove this call from Inet6AddressImpl and add it to the test instead. Adding
the call to jdk.internal.loader.BootLoader.loadLibrary("net"); in the test
should work since the actual test class (NullCharInHostname.java) is injected
in java.base.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20317#discussion_r1711706176
Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava [v5]
On Fri, 9 Aug 2024 17:59:12 GMT, Brian Burkhalter wrote: >> This proposed change would move the native objects required for NIO file >> interaction from the libnio native library to the libjava native library on >> Linux, macOS, and Windows. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8337143: Remove loading libnet from Inet6AddressImpl; delete commented out > #include in Windows IOUtil.c The changes to the java/net code look OK to me now. Thanks Brian. I am approving these changes - but please also get a Reviewer for the NIO and build side of these. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20317#pullrequestreview-2233341338
Re: RFR: 8338445: jdk.internal.loader.URLClassPath may leak JarFile instance when dealing with unexpected Class-Path entry in manifest [v2]
On Mon, 26 Aug 2024 15:17:18 GMT, Michael McMahon wrote:
>> Jaikiran Pai has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - revert to old code comment
>> - use "JAR file" consistently in test's code comments
>> - rename closeLoaderIgnoreEx to closeQuietly
>
> src/java.base/share/classes/jdk/internal/loader/URLClassPath.java line 470:
>
>> 468: continue;
>> 469: }
>> 470: if (loaderClassPathURLs != null) {
>
> I'm missing something here. Why does the compiler not complain that
> `loaderClassPathURLs` might not be initialized, when it's accessed here?
Because we would not reach here - since it's either assigned at line 447 or we
continue at line 457, 468, or exit throwing an uncaught exception?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20691#discussion_r1731421835
Re: RFR: 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni [v2]
On Tue, 27 Sep 2022 23:12:43 GMT, Michael Ernst wrote: > Feel free to break up the pull request if that is what is needed to free it > from red tape. Only you can do that @mernst - PR: https://git.openjdk.org/jdk/pull/10029
Re: RFR: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited [v2]
On Tue, 27 Sep 2022 12:14:23 GMT, Pavel Rappo wrote:
>> This adds exception documentation to JDK methods that would otherwise lose
>> that documentation once JDK-8287796 is integrated. While adding this
>> exception documentation now does not change [^1] the JDK API Documentation,
>> it will automatically counterbalance the effect that JDK-8287796 will have.
>>
>> JDK-8287796 seeks to remove an old, undocumented, and esoteric javadoc
>> feature that cannot be suppressed [^2]. The feature is so little known about
>> that IDEs either do not implement it correctly or do not implement it at
>> all, thus rendering documentation differently from how the javadoc tool
>> renders it.
>>
>> That feature was introduced in JDK-4947455 and manifests as follows. If a
>> method inherits documentation for an exception, along with that
>> documentation the method automatically inherits documentation for all
>> exceptions that are subclasses of that former exception and are documented
>> in an overridden method.
>>
>> To illustrate that behavior, consider the following example. A method
>> `Subtype.m` inherits documentation for `SuperException`, while the
>> overridden method `Supertype.m` documents `SuperException`, `SubExceptionA`
>> and `SubExceptionB`, where the latter two exceptions are subclasses of the
>> former exception:
>>
>> public class Subtype extends Supertype {
>>
>> @Override
>> public void m() throws SuperException {
>> ...
>>
>> public class Supertype {
>>
>> /**
>> * @throws SuperException general problem
>> * @throws SubExceptionA specific problem A
>> * @throws SubExceptionB specific problem B
>> */
>> public void m() throws SuperException {
>> ...
>>
>> public class SuperException extends Exception {
>> ...
>>
>> public class SubExceptionA extends SuperException {
>> ...
>>
>> public class SubExceptionB extends SuperException {
>> ...
>>
>> For the above example, the API Documentation for `Subtype.m` will contain
>> documentation for all three exceptions; the page fragment for `Subtype.m` in
>> "Method Details" will look like this:
>>
>> public void m()
>>throws SuperException
>>
>> Overrides:
>> m in class Supertype
>> Throws:
>> SuperException - general problem
>> SubExceptionA - specific problem A
>> SubExceptionB - specific problem B
>>
>> It works for checked and unchecked exceptions, for methods in classes and
>> interfaces.
>>
>>
>> If it's the first time you hear about that behavior and this change affects
>> your area, it's a good opportunity to reflect on whether the exception
>> documentation this change adds is really needed or you were simply unaware
>> of the fact that it was automatically added before. If exception
>> documentation is not needed, please comment on this PR. Otherwise, consider
>> approving it.
>>
>> Keep in mind that if some exception documentation is not needed, **leaving
>> it out** from this PR might require a CSR.
>>
>>
>> [^1]: Aside from insignificant changes on class-use and index pages. There's
>> one relatively significant change. This change is in jdk.jshell, where some
>> methods disappeared from "Methods declared in ..." section and other
>> irregularities. We are aware of this and have filed JDK-8291803 to fix the
>> root cause.
>> [^2]: In reality, the feature can be suppressed, but it looks like a bug
>> rather than intentional design. If we legitimize the feature and its
>> suppression behavior, the model for exception documentation inheritance
>> might become much more complicated.
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> revert an extraneous change to jdk.javadoc
The changes to JNDI and NIO look reasonable to me - though I haven't verified
whether anything was missing (or no longer needed). If you can assert that the
generated docs are the same before and after the proposed change then that's
enough for me.
src/java.naming/share/classes/javax/naming/directory/InitialDirContext.java
line 186:
> 184: /**
> 185: * @throws AttributeModificationException {@inheritDoc}
> 186: */
Isn't `{@inheritDoc}` needed in this case to preserve the method & params
description?
-
PR: https://git.openjdk.org/jdk/pull/10449
Re: RFR: 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni [v2]
On Wed, 28 Sep 2022 14:45:54 GMT, Michael Ernst wrote: >> Michael Ernst has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains six commits: >> >> - Reinstate typos in Apache code that is copied into the JDK >> - Merge ../jdk-openjdk into typos-typos >> - Remove file that was removed upstream >> - Fix inconsistency in capitalization >> - Undo change in zlip >> - Fix typos > > Regarding breaking up the pull request: > * No one has stated that this is actually necessary or given a guarantee > that it will result in a merged pull request, only that it *might* finally > free this pull request of red tape. > * Despite my repeated requests, no one has justified that *this particular > pull request* needs the expertise of different teams. The only reasons > stated have been custom and bureaucracy. > * I have spent a nontrivial amount of time on these fixes. There have been > long delays, which forced me to resolve merge conflicts. I was told that the > changes are "mostly fine" without any indication of what is wrong. I am > reluctant to spend yet more time, only to later (maybe weeks later, as has > already happened) find out that I still have to do something else or that all > my effort was wasted. > * I was condescendingly told "We have discussed several times in the past > ..." even though I was not privy to those discussions. No pointer to > documentation in the JDK itself, about pull requests, was given. > * No one has explained what to do. "separate PRs by team" doesn't tell me > what to do, since I don't see an org chart anywhere in the documentation that > tells me what a "team" is. > > I'm sorry to state it so bluntly, but this is not a good look for the JDK > team being welcoming to contributions. Hi @mernst, sorry if this is how it appears. Pull requests that involve changes spanning a large set of files are notoriously harder to review since they require a reviewer from each different area to chime in. Basically you will need one reviewer from each of these areas: - client - compiler - core-libs - hotspot - i18n - javadoc - jmx - net - nio - security - serviceability Jaikiran suggested to reduce the scope of this PR in his first comment: https://github.com/openjdk/jdk/pull/10029#issuecomment-1236588632 as did several other reviewers. Sorry if the meaning (or reason) was not clear. best regards, - PR: https://git.openjdk.org/jdk/pull/10029
Re: RFR: 8294456: Fix misleading-indentation warnings in JDK
On Thu, 29 Sep 2022 13:11:03 GMT, Raffaello Giulietti wrote: > This fixes misleading indentations, which allows enabling the (currently > disabled) `misleading-indentation` warning flag on two `.gmk` files. src/java.base/share/native/libfdlibm/e_asin.c line 102: > 100: } else > 101: t = x*x; > 102: p = t*(pS0+t*(pS1+t*(pS2+t*(pS3+t*(pS4+t*pS5); should we add an opening brace after the `else` (and close it before line 102) to make it more obvious that there is no problem with the indentation? - PR: https://git.openjdk.org/jdk/pull/10487
Re: RFR: 8294456: Fix misleading-indentation warnings in JDK
On Thu, 29 Sep 2022 13:44:40 GMT, Raffaello Giulietti wrote: >> src/java.base/share/native/libfdlibm/e_asin.c line 102: >> >>> 100: } else >>> 101: t = x*x; >>> 102: p = t*(pS0+t*(pS1+t*(pS2+t*(pS3+t*(pS4+t*pS5); >> >> should we add an opening brace after the `else` (and close it before line >> 102) to make it more obvious that there is no problem with the indentation? > > It seems that this code is almost a verbatim copy of the upstream code > available on the indicated link. > So, while I agree that it would be better to add braces, it would make the > code diverge from the upstream source, even if the change would be benign. > and welcome OK - that's a reasonable concern indeed. - PR: https://git.openjdk.org/jdk/pull/10487
Re: RFR: 8293940: Some tests for virtual threads take too long
On Wed, 28 Sep 2022 08:07:25 GMT, Alan Bateman wrote: > This is a test only change to split the execution of some of the larger tests > for virtual threads and reduce the execution time of a few others. > > The tests StructuredTaskScopeTest, ThreadFockTest and > ThreadPerTaskExecutorTest contain a lot of tests and run with thread > factories for both platform and virtual threads. These tests are changed to > run twice, one with the thread factory for platform threads, the other for > virtual threads. This allows the tests to run concurrently. A number of > sleeps/delays in these tests are adjusted. StructuredTaskScopeTest goes from > ~50s to two runs of ~15s. ThreadFockTest goes from 39s to two runs of 15s, > ThreadPerTaskExecutorTest goes from 21s to two runs of 8.5. There is more > that could be done but these tests are likely to change a bit the APIs evolve > so it may not be worth doing now. > > ThreadAPI also contains a lot of tests. A number of tests sleep to give time > for a virtual thread to park or block. These sleeps are replaced with a poll > of the thread state. A number of other tests are changed to use shorter > sleeps. There are a few cleanups that include introducing a number of data > providers and using a STPE for delayed actions. Its execution time goes from > ~31s to ~12s. > > CloseTest has two tests that sleep 1s and they are run with a data provider > that produces six inputs so the sleep time adds up. These lengthy delays > aren't needed for testing > > Finally, some of the stress changes have been changed. The two runs of > PingPong are changed so they are in separate @test descriptions and the > iteration count on a few tests has been reduced for release builds - the > iteration count for debug builds is not changed. I have gone through the proposed changes and they look reasonable. I can't comment on the new threshold values which I assume have been obtained from careful experimentation. If that makes the tests faster and if you checked that they remain stable in the CI then this looks like a good change! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10463
Re: RFR: 8294697: java/lang/Thread/virtual/ThreadAPI.testGetStackTrace2 failed with non-empty stack trace
On Wed, 5 Oct 2022 14:05:38 GMT, Alan Bateman wrote: > This is a test-only change. ThreadAPI.testGetStackTrace2 tests the stack > trace of a virtual thread that has started but has not executed it. The test > currently attempts to saturate all carrier threads before starting a target > thread. The test is not reliable and failed at least once with a debug build > and -Xcomp. The test is changed to use a custom scheduler that discards the > task so the thread doesn't execute. The resulting test is much simpler and > removes 3s from the execution time. This looks reasonable to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10576
Re: RFR: 8293810: Remove granting of RuntimePermission("stopThread") from tests
On Wed, 5 Oct 2022 15:01:15 GMT, Alan Bateman wrote:
> This is a test only change to remove the granting of
> RuntimePermission("stopThread") from the tests. With Thread.stop changed to
> throw UOE it means there is nothing that requires this permission.
Changes to java.net and javax.management look fine to me.
-
Marked as reviewed by dfuchs (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10577
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation
On Wed, 5 Oct 2022 15:23:43 GMT, Aleksei Efimov wrote: > ### Summary of the change > This change introduces new system and security properties for specifying > factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider > implementations. > > These new properties allow more granular control over the set of object > factories allowed to reconstruct Java objects from LDAP and RMI contexts. > > The new factory filters are supplementing the existing > `jdk.jndi.object.factoriesFilter` global factories filter to determine if a > specific object factory is permitted to instantiate objects for the given > protocol. > > Links: > > - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) > - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) > > ### List of code changes > > - Implementation for two new system and security properties have been added > to the `com.sun.naming.internal.ObjectFactoriesFilter` class > - `java.security` and `module-info.java` files have been updated with a > documentation for the new properties > - To keep API of `javax.naming.spi.NamingManager` and > `javax.naming.spi.DirectoryManager` classes unmodified a new internal > `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All > `getObjectInstance` calls have been updated to use the new helper class. > > NamingManagerHelper changes > Changes performed to construct the `NamingManagerHelper` class: > - `DirectoryManager.getObjectInstance` -> > `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also moved > to the `NamingManagerHelper` class > - `NamingManager.getObjectInstance` -> > `NamingManagerHelper.getObjectInstance`. Methods responsible for > setting/getting object factory builder were moved to the > `NamingManagerHelper` class too. > > > ### Test changes > New tests have been added for checking that new factory filters can be used > to restrict reconstruction of Java objects from LDAP and RMI contexts: > - LDAP protocol specific test: > test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java > - RMI protocol specific test: > test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java > Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated > to allow test-specific factories filter used to reconstruct objects from the > test LDAP server. > > ### Testing > tier1-tier3 and JNDI regression/JCK tests not showing any failures related to > this change. > No failures observed for the modified regression tests. Marked as reviewed by dfuchs (Reviewer). I am glad to see this RFE. It looks like a big change but most of it is actually reorganisation of internal code, so thanks for explaining its making off :-) It helps a lot for the review. I have looked at the code and I believe it looks good. I don't see any alternative to the reorganisation that could make the changes smaller - so I'm OK with the proposed solution. Thanks for documenting the new properties in their respective module info, as well as in the security properties file. I had only a cursory look at the tests but they seem comprehensive. Good work. I am approving on the condition to make sure that all JNDI tests (as well as tier1, tier2, tier3) are run before integrating. Also please get an approval from a security-libs maintainer for the changes to the security properties file before integrating. - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v2]
On Sun, 9 Oct 2022 11:52:18 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> This change introduces new system and security properties for specifying >> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider >> implementations. >> >> These new properties allow more granular control over the set of object >> factories allowed to reconstruct Java objects from LDAP and RMI contexts. >> >> The new factory filters are supplementing the existing >> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a >> specific object factory is permitted to instantiate objects for the given >> protocol. >> >> Links: >> >> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) >> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) >> >> ### List of code changes >> >> - Implementation for two new system and security properties have been added >> to the `com.sun.naming.internal.ObjectFactoriesFilter` class >> - `java.security` and `module-info.java` files have been updated with a >> documentation for the new properties >> - To keep API of `javax.naming.spi.NamingManager` and >> `javax.naming.spi.DirectoryManager` classes unmodified a new internal >> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All >> `getObjectInstance` calls have been updated to use the new helper class. >> >> NamingManagerHelper changes >> Changes performed to construct the `NamingManagerHelper` class: >> - `DirectoryManager.getObjectInstance` -> >> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also >> moved to the `NamingManagerHelper` class >> - `NamingManager.getObjectInstance` -> >> `NamingManagerHelper.getObjectInstance`. Methods responsible for >> setting/getting object factory builder were moved to the >> `NamingManagerHelper` class too. >> >> >> ### Test changes >> New tests have been added for checking that new factory filters can be used >> to restrict reconstruction of Java objects from LDAP and RMI contexts: >> - LDAP protocol specific test: >> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java >> - RMI protocol specific test: >> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java >> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated >> to allow test-specific factories filter used to reconstruct objects from the >> test LDAP server. >> >> ### Testing >> tier1-tier3 and JNDI regression/JCK tests not showing any failures related >> to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov 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: > > - Refactor checkInput, better reporting for invalid filter patterns > - Merge branch 'master' into JDK-8290368_protocol_specific_factory_filters > - Additional comments/formatting cleanup. > - More tests clean-up. Code/doc comments cleanup. > - Cleanup test comments. Add tests to check that LDAP/RMI filters do not > intersect. > - 8290368: Introduce LDAP and RMI protocol-specific object factory filters > to JNDI implementation Changes requested by dfuchs (Reviewer). src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java line 99: > 97: return globalResult == Status.ALLOWED; > 98: } > 99: If I'm not mistaken there's no point in checking the specific filter if the global filter state is REJECTED. So instead of switching on the specificResult below, maybe you should change the logic to switch on the globalResult instead and only call the specific filter if needed? - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v2]
On Mon, 10 Oct 2022 12:07:38 GMT, Aleksei Efimov wrote:
>> src/java.naming/share/classes/com/sun/naming/internal/ObjectFactoriesFilter.java
>> line 99:
>>
>>> 97: return globalResult == Status.ALLOWED;
>>> 98: }
>>> 99:
>>
>> If I'm not mistaken there's no point in checking the specific filter if the
>> global filter state is REJECTED. So instead of switching on the
>> specificResult below, maybe you should change the logic to switch on the
>> globalResult instead and only call the specific filter if needed?
>
>> If I'm not mistaken there's no point in checking the specific filter if the
>> global filter state is REJECTED. So instead of switching on the
>> specificResult below, maybe you should change the logic to switch on the
>> globalResult instead and only call the specific filter if needed?
>
> Yes - there is no point, and that will reduce number of `checkInput` calls on
> a specific filter, if it is not the global one. That's how it will look like:
>
> private static boolean checkInput(ConfiguredFilter filter, FactoryInfo
> serialClass) {
> var globalFilter = GLOBAL_FILTER.filter();
> var specificFilter = filter.filter();
> Status globalResult = globalFilter.checkInput(serialClass);
>
> // Check if a specific filter is the global one
> if (filter == GLOBAL_FILTER) {
> return globalResult == Status.ALLOWED;
> }
> return switch (globalResult) {
> case ALLOWED -> specificFilter.checkInput(serialClass) !=
> Status.REJECTED;
> case REJECTED -> false;
> case UNDECIDED -> specificFilter.checkInput(serialClass) ==
> Status.ALLOWED;
> };
> }
>
>
> The `if (filter == GLOBAL_FILTER) {` check can be also removed but without it
> the `checkInput` will be called twice on global filter for the case where
> `specific == global`, ie call from the `checkGlobalFilter`.
The code above LGTM. An alternative could be:
private static boolean checkInput(ConfiguredFilter filter, FactoryInfo
serialClass) {
var globalFilter = GLOBAL_FILTER.filter();
var specificFilter = filter.filter();
Status globalResult = globalFilter.checkInput(serialClass);
return switch (globalResult) {
case ALLOWED -> filter == GLOBAL ||
specificFilter.checkInput(serialClass) != Status.REJECTED;
case REJECTED -> false;
case UNDECIDED -> specificFilter.checkInput(serialClass) ==
Status.ALLOWED;
};
}
but I believe your proposed code reads better.
-
PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v3]
On Mon, 10 Oct 2022 14:28:07 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> This change introduces new system and security properties for specifying >> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider >> implementations. >> >> These new properties allow more granular control over the set of object >> factories allowed to reconstruct Java objects from LDAP and RMI contexts. >> >> The new factory filters are supplementing the existing >> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a >> specific object factory is permitted to instantiate objects for the given >> protocol. >> >> Links: >> >> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) >> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) >> >> ### List of code changes >> >> - Implementation for two new system and security properties have been added >> to the `com.sun.naming.internal.ObjectFactoriesFilter` class >> - `java.security` and `module-info.java` files have been updated with a >> documentation for the new properties >> - To keep API of `javax.naming.spi.NamingManager` and >> `javax.naming.spi.DirectoryManager` classes unmodified a new internal >> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All >> `getObjectInstance` calls have been updated to use the new helper class. >> >> NamingManagerHelper changes >> Changes performed to construct the `NamingManagerHelper` class: >> - `DirectoryManager.getObjectInstance` -> >> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also >> moved to the `NamingManagerHelper` class >> - `NamingManager.getObjectInstance` -> >> `NamingManagerHelper.getObjectInstance`. Methods responsible for >> setting/getting object factory builder were moved to the >> `NamingManagerHelper` class too. >> >> >> ### Test changes >> New tests have been added for checking that new factory filters can be used >> to restrict reconstruction of Java objects from LDAP and RMI contexts: >> - LDAP protocol specific test: >> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java >> - RMI protocol specific test: >> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java >> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated >> to allow test-specific factories filter used to reconstruct objects from the >> test LDAP server. >> >> ### Testing >> tier1-tier3 and JNDI regression/JCK tests not showing any failures related >> to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Change checkInput to be the global filter centric Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v5]
On Fri, 14 Oct 2022 16:19:41 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> This change introduces new system and security properties for specifying >> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider >> implementations. >> >> These new properties allow more granular control over the set of object >> factories allowed to reconstruct Java objects from LDAP and RMI contexts. >> >> The new factory filters are supplementing the existing >> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a >> specific object factory is permitted to instantiate objects for the given >> protocol. >> >> Links: >> >> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) >> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) >> >> ### List of code changes >> >> - Implementation for two new system and security properties have been added >> to the `com.sun.naming.internal.ObjectFactoriesFilter` class >> - `java.security` and `module-info.java` files have been updated with a >> documentation for the new properties >> - To keep API of `javax.naming.spi.NamingManager` and >> `javax.naming.spi.DirectoryManager` classes unmodified a new internal >> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All >> `getObjectInstance` calls have been updated to use the new helper class. >> >> NamingManagerHelper changes >> Changes performed to construct the `NamingManagerHelper` class: >> - `DirectoryManager.getObjectInstance` -> >> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also >> moved to the `NamingManagerHelper` class >> - `NamingManager.getObjectInstance` -> >> `NamingManagerHelper.getObjectInstance`. Methods responsible for >> setting/getting object factory builder were moved to the >> `NamingManagerHelper` class too. >> >> >> ### Test changes >> New tests have been added for checking that new factory filters can be used >> to restrict reconstruction of Java objects from LDAP and RMI contexts: >> - LDAP protocol specific test: >> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java >> - RMI protocol specific test: >> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java >> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated >> to allow test-specific factories filter used to reconstruct objects from the >> test LDAP server. >> >> ### Testing >> tier1-tier3 and JNDI regression/JCK tests not showing any failures related >> to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov 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 ten additional > commits since the last revision: > > - Merge branch 'master' into JDK-8290368_protocol_specific_factory_filters > - Remove factory builder synchronization from NamingManager. Update > comments/docs. > - Change checkInput to be the global filter centric > - Refactor checkInput, better reporting for invalid filter patterns > - Merge branch 'master' into JDK-8290368_protocol_specific_factory_filters > - Additional comments/formatting cleanup. > - More tests clean-up. Code/doc comments cleanup. > - Cleanup test comments. Add tests to check that LDAP/RMI filters do not > intersect. > - 8290368: Introduce LDAP and RMI protocol-specific object factory filters > to JNDI implementation src/java.base/share/conf/security/java.security line 1388: > 1386: # are unused. > 1387: # > 1388: # Each class name pattern is matched against the factory class name to > allow or disallow its It appears that for those protocols for which there is no specific filter, a factory class will be accepted only if the global filter returns ALLOWED - which contradicts what is written here (where it says that the class is allowed if it's not REJECTED). Is this something that has changed with this fix - or was the documentation wrong before? - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation [v6]
On Mon, 17 Oct 2022 15:32:55 GMT, Aleksei Efimov wrote: >> ### Summary of the change >> This change introduces new system and security properties for specifying >> factory filters for the JNDI/LDAP and the JNDI/RMI JDK provider >> implementations. >> >> These new properties allow more granular control over the set of object >> factories allowed to reconstruct Java objects from LDAP and RMI contexts. >> >> The new factory filters are supplementing the existing >> `jdk.jndi.object.factoriesFilter` global factories filter to determine if a >> specific object factory is permitted to instantiate objects for the given >> protocol. >> >> Links: >> >> - [CSR with details](https://bugs.openjdk.org/browse/JDK-8291556) >> - [JBS issue](https://bugs.openjdk.org/browse/JDK-8290368) >> >> ### List of code changes >> >> - Implementation for two new system and security properties have been added >> to the `com.sun.naming.internal.ObjectFactoriesFilter` class >> - `java.security` and `module-info.java` files have been updated with a >> documentation for the new properties >> - To keep API of `javax.naming.spi.NamingManager` and >> `javax.naming.spi.DirectoryManager` classes unmodified a new internal >> `com.sun.naming.internal.NamingManagerHelper` class has been introduced. All >> `getObjectInstance` calls have been updated to use the new helper class. >> >> NamingManagerHelper changes >> Changes performed to construct the `NamingManagerHelper` class: >> - `DirectoryManager.getObjectInstance` -> >> `NamingManagerHelper.getDirObjectInstance`. Dependant methods were also >> moved to the `NamingManagerHelper` class >> - `NamingManager.getObjectInstance` -> >> `NamingManagerHelper.getObjectInstance`. Methods responsible for >> setting/getting object factory builder were moved to the >> `NamingManagerHelper` class too. >> >> >> ### Test changes >> New tests have been added for checking that new factory filters can be used >> to restrict reconstruction of Java objects from LDAP and RMI contexts: >> - LDAP protocol specific test: >> test/jdk/com/sun/jndi/ldap/objects/factory/LdapFactoriesFilterTest.java >> - RMI protocol specific test: >> test/jdk/com/sun/jndi/rmi/registry/objects/RmiFactoriesFilterTest.java >> Existing `test/jdk/javax/naming/module/RunBasic.java` test has been updated >> to allow test-specific factories filter used to reconstruct objects from the >> test LDAP server. >> >> ### Testing >> tier1-tier3 and JNDI regression/JCK tests not showing any failures related >> to this change. >> No failures observed for the modified regression tests. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Update filter docs to match implementation. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10578
Re: RFR: 8295823: Use enhanced-for cycle instead of Enumeration in java.naming
On Thu, 20 Oct 2022 20:28:37 GMT, Andrey Turbanov wrote: > `java.util.Enumeration` is a legacy interface from java 1.0. > There are couple of cycles which use it to iterate over collections. We can > replace this manual cycle with enchanced-for, which is shorter and easier to > read. LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10804
RFR: 8294241: Deprecate URL public constructors
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
-
Commit messages:
- Fix whitespace issues
- 8294241
Changes: https://git.openjdk.org/jdk/pull/10874/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8294241
Stats: 849 lines in 82 files changed: 701 ins; 2 del; 146 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
On Wed, 26 Oct 2022 17:09:20 GMT, Xue-Lei Andrew Fan 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
>
> src/java.base/share/classes/java/net/URL.java line 852:
>
>> 850: * @since 20
>> 851: */
>> 852: public static URL fromURI(URI uri, URLStreamHandler streamHandler)
>
> What do you think to have this method in URI instead:
> URI.toURL(URLStreamHandler), as there is an URI.toURL() method already?
`URLStreamHandler` really belongs to `java.net.URL`, and is an implementation
detail of the infrastructure/SPI that makes it possible to eventually call
`URL::openConnection`. I do not think it would be appropriate to have such a
method in `java.net.URI`. Dealing with `URLStreamHandler` is advanced usage and
is not something that a regular developer will need to do, and it is better if
it isn't too prominent.
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors
On Wed, 26 Oct 2022 17:39:56 GMT, Xue-Lei Andrew Fan wrote: >> `URLStreamHandler` really belongs to `java.net.URL`, and is an >> implementation detail of the infrastructure/SPI that makes it possible to >> eventually call `URL::openConnection`. I do not think it would be >> appropriate to have such a method in `java.net.URI`. Dealing with >> `URLStreamHandler` is advanced usage and is not something that a regular >> developer will need to do, and it is better if it isn't too prominent. > > I see your point. It may be more appropriate if URI.toURL was designed as > URL.fromURL. > > I was wondering to have application developers a consistent way to get an URL > instance. Now there are two methods in different classes URI.toURL and > URL.fromURI. It might be easier to use the old URI.toURL form. > > Never mind, it is just my personal preference. It is fine to me to have a > new URL.fromURI method. One thing we might do is change the name of the method into `URL.of(URI, StreamHandler)`. It's named `fromURI` merely because there was a pre-existing package protected `fromURI` method. However since we're making it public now, it could be fair game to change its name. Possibly adding an overload `URL::of(URI)` method could be considered, but then there really would be two paths to do the same thing - so I'd rather not add such an overload - unless I get some more feedback on that from the CSR/PR review. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors
On Thu, 27 Oct 2022 09:17:29 GMT, Michael McMahon wrote: >> Having unnamed local variables[^1] would probably be best for this. >> >> [^1]: https://openjdk.org/jeps/8294349 > > How about `_unused` or `_unused1`, `_unused2` then in the meantime? I'd be happy to make the change. Let's wait to see if anybody has a better naming suggestion. - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors
On 27/10/2022 07:26, Alan Bateman wrote:
We have a strict URI 3986 implementation, which we use to create all
URL instances from.
Note also that though this change proposes to deprecate these
constructors in order to provide a stronger warning against their
potential misuse, it does not deprecate them for removal.
If you are confident in your parsing/validation you can of course
still provide a `URL toURL(org.apache.river.api.net.URI)` method that
calls a constructor, and annotate that one place with
`@SuppressWarnings("deprecation")`.
best regards,
-- dabniel
Re: RFR: 8294241: Deprecate URL public constructors
On Thu, 27 Oct 2022 17:20:04 GMT, Joe Wang wrote: > Hi Daniel, if it's not a major improvement, we'd like to keep the java.xml > module at the JDK 8 code level. Can we remove the 'var' usage in a few > java.xml classes? No problem - I will make this change when we have settled on a name for the variable `tmp` vs `_unused` (proposed by Michael). - PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors
On Thu, 27 Oct 2022 17:50:37 GMT, Andrey Turbanov 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
>
> test/jdk/java/net/URL/URLFromURITest.java line 268:
>
>> 266: // - URL authority is null or empty depending on the
>> protocol
>> 267: // and on whether the URL is hierarchical or not.
>> 268: if (isFileBased && authority == null) {
>
> Suggestion:
>
> if (isFileBased && authority == null) {
Thanks - I will apply this suggestion.
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v2]
> 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 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
-
Changes:
- all: https://git.openjdk.org/jdk/pull/10874/files
- new: https://git.openjdk.org/jdk/pull/10874/files/25a0188c..fd4ca287
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=10874&range=00-01
Stats: 25761 lines in 392 files changed: 1581 ins; 23649 del; 531 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: 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
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: 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 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: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: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 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 [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: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 [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 [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 [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 [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
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: 8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly [v2]
On Mon, 7 Nov 2022 19:28:56 GMT, Aleksei Efimov wrote:
>> ### The Proposed Change
>>
>> The proposed change updates JNDI's `DnsClient` internal implementation to
>> use `DatagramChannel` (DC) as a replacement for `DatagramSocket` (DS).
>> The main motivation behind this change is to make JNDI/DNS lookups friendly
>> to virtual-thread environments and update its underlying implementation to
>> use efficient `DatagramChannel` APIs.
>> The list of proposed changes:
>> - Replace DS usage with DC. That includes the `DNSDatagramSocketFactory`
>> class updates to return DC instead of DS. The factory class was renamed to
>> `DNSDatagramChannelFactory` to reflect that.
>> - Change DNS query timeouts implementation - the current version introduces
>> a nio channels selector to implement timeouts. One selector is created for
>> each instance of `DnsClient`.
>> - Adjust query retries logic to the new implementation of timeouts.
>> - Modify the `Timeout` test to create a bound UDP socket to simulate an
>> unresponsive DNS server. Before this change, the test was using the
>> '10.0.0.0' network address that doesn't belong to any host. The proposed
>> change with a bound unresponsive UDP socket is better for test stability on
>> different platforms.
>>
>>
>> ### Testing
>> `jdk-tier1` to `jdk-tier3 `tests are showing no failures related to the
>> changes.
>> JNDI regression and JCK tests also didn't highlight any problems with the
>> changes.
>>
>> Also, an app performing a DNS lookup from a virtual thread context executed
>> with the following options `--enable-preview -Djdk.tracePinnedThreads=full`
>> showed no pinned carrier threads. Before the proposed change the following
>> pinned stack trace was observed:
>> ```
>> java.base/sun.nio.ch.DatagramChannelImpl.park(DatagramChannelImpl.java:486)
>>
>> java.base/sun.nio.ch.DatagramChannelImpl.trustedBlockingReceive(DatagramChannelImpl.java:734)
>>
>> java.base/sun.nio.ch.DatagramChannelImpl.blockingReceive(DatagramChannelImpl.java:661)
>>
>> java.base/sun.nio.ch.DatagramSocketAdaptor.receive(DatagramSocketAdaptor.java:241)
>> <== monitors:1
>> java.base/java.net.DatagramSocket.receive(DatagramSocket.java:714)
>> jdk.naming.dns/com.sun.jndi.dns.DnsClient.doUdpQuery(DnsClient.java:430)
>> <== monitors:1
>> jdk.naming.dns/com.sun.jndi.dns.DnsClient.query(DnsClient.java:216)
>> jdk.naming.dns/com.sun.jndi.dns.Resolver.query(Resolver.java:81)
>> jdk.naming.dns/com.sun.jndi.dns.DnsContext.c_lookup(DnsContext.java:290)
>>
>> java.naming/com.sun.jndi.toolkit.ctx.ComponentContext.p_lookup(ComponentContext.java:542)
>>
>> java.naming/com.sun.jndi.toolkit.ctx.PartialCompositeContext.lookup(PartialCompositeContext.java:177)
>>
>> java.naming/com.sun.jndi.toolkit.ctx.PartialCompositeContext.lookup(PartialCompositeContext.java:166)
>> java.naming/javax.naming.InitialContext.lookup(InitialContext.java:409)
>>
>> After proposed changes - pinned threads are not detectable.
>
> Aleksei Efimov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
Changes requested by dfuchs (Reviewer).
src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DNSDatagramChannelFactory.java
line 279:
> 277: } catch (IOException x) {
> 278: // try again until maxtries == 0;
> 279: }
That can be simplified now:
DatagramChannel dc = (family != null)
? DatagramChannel.open(family)
: DatagramChannel.open();
try {
dc.bind(new InetSocketAddress(port));
lastport = getLocalPort(dc);
if (!recycled) history.add(port);
return dc;
} catch (IOException x) {
dc.close();
// try again until maxtries == 0;
}
There's probably no need to retry if the open call fails, as the next call to
`open` is likely to fail too. So if `open` throws we should probably let it
propagate upwards - (and declare openRandom() to throw IOException).
-
PR: https://git.openjdk.org/jdk/pull/11007
Re: RFR: 8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly [v2]
On Mon, 7 Nov 2022 15:39:55 GMT, Aleksei Efimov wrote: >> src/jdk.naming.dns/share/classes/com/sun/jndi/dns/DnsClient.java line 470: >> >>> 468: } while (timeoutLeft > MIN_TIMEOUT); >>> 469: // no matching packet received within the timeout >>> 470: throw new SocketTimeoutException(); >> >> It appears to me that, before the change in this PR, we used to return >> `null` from this method if there is a timeout. The calling code (the method >> `query`) would then interpret this `null` return in a couple of different >> ways. One of them being, skipping this server and querying any other >> server(s) that were known to the client instance. Now, with this change >> where we throw this `SocketTimeoutException`, that part of the code would >> behave differently from what I can see. Is this intentional to throw an >> exception instead of returning `null`? > > Thanks for spotting that, Jai. > You're correct that in its current version the fix changed an old logic of > `doUdpQuery`/`query` methods: > Before this change the method was returning `null` not when a receive is > timed out but when an unmatched packet is received. Socket timeout exceptions > thrown by `DatagramSocket.receive` were caught in `query` method. > After the proposed change the `doUdpQuery` method is throwing > `SocketTimeoutException` for both cases (timeout and unmatched packets) - > that needs to be changed to comply with old logic. Will address it in an > upcoming changeset. This is addressed now - right? - PR: https://git.openjdk.org/jdk/pull/11007
Re: RFR: 8290714: Make com.sun.jndi.dns.DnsClient virtual threads friendly [v3]
On Tue, 8 Nov 2022 00:26:56 GMT, Aleksei Efimov wrote: >> ### The Proposed Change >> >> The proposed change updates JNDI's `DnsClient` internal implementation to >> use `DatagramChannel` (DC) as a replacement for `DatagramSocket` (DS). >> The main motivation behind this change is to make JNDI/DNS lookups friendly >> to virtual-thread environments and update its underlying implementation to >> use efficient `DatagramChannel` APIs. >> The list of proposed changes: >> - Replace DS usage with DC. That includes the `DNSDatagramSocketFactory` >> class updates to return DC instead of DS. The factory class was renamed to >> `DNSDatagramChannelFactory` to reflect that. >> - Change DNS query timeouts implementation - the current version introduces >> a nio channels selector to implement timeouts. One selector is created for >> each instance of `DnsClient`. >> - Adjust query retries logic to the new implementation of timeouts. >> - Modify the `Timeout` test to create a bound UDP socket to simulate an >> unresponsive DNS server. Before this change, the test was using the >> '10.0.0.0' network address that doesn't belong to any host. The proposed >> change with a bound unresponsive UDP socket is better for test stability on >> different platforms. >> >> >> ### Testing >> `jdk-tier1` to `jdk-tier3 `tests are showing no failures related to the >> changes. >> JNDI regression and JCK tests also didn't highlight any problems with the >> changes. >> >> Also, an app performing a DNS lookup from a virtual thread context executed >> with the following options `--enable-preview -Djdk.tracePinnedThreads=full` >> showed no pinned carrier threads. Before the proposed change the following >> pinned stack trace was observed: >> ``` >> java.base/sun.nio.ch.DatagramChannelImpl.park(DatagramChannelImpl.java:486) >> >> java.base/sun.nio.ch.DatagramChannelImpl.trustedBlockingReceive(DatagramChannelImpl.java:734) >> >> java.base/sun.nio.ch.DatagramChannelImpl.blockingReceive(DatagramChannelImpl.java:661) >> >> java.base/sun.nio.ch.DatagramSocketAdaptor.receive(DatagramSocketAdaptor.java:241) >> <== monitors:1 >> java.base/java.net.DatagramSocket.receive(DatagramSocket.java:714) >> jdk.naming.dns/com.sun.jndi.dns.DnsClient.doUdpQuery(DnsClient.java:430) >> <== monitors:1 >> jdk.naming.dns/com.sun.jndi.dns.DnsClient.query(DnsClient.java:216) >> jdk.naming.dns/com.sun.jndi.dns.Resolver.query(Resolver.java:81) >> jdk.naming.dns/com.sun.jndi.dns.DnsContext.c_lookup(DnsContext.java:290) >> >> java.naming/com.sun.jndi.toolkit.ctx.ComponentContext.p_lookup(ComponentContext.java:542) >> >> java.naming/com.sun.jndi.toolkit.ctx.PartialCompositeContext.lookup(PartialCompositeContext.java:177) >> >> java.naming/com.sun.jndi.toolkit.ctx.PartialCompositeContext.lookup(PartialCompositeContext.java:166) >> java.naming/javax.naming.InitialContext.lookup(InitialContext.java:409) >> >> After proposed changes - pinned threads are not detectable. > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Simplify openRandom Changes look good! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11007
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
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: 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: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops [v12]
On Thu, 10 Nov 2022 15:03:26 GMT, Claes Redestad wrote: >> Continuing the work initiated by @luhenry to unroll and then intrinsify >> polynomial hash loops. >> >> I've rewired the library changes to route via a single `@IntrinsicCandidate` >> method. To make this work I've harmonized how they are invoked so that >> there's less special handling and checks in the intrinsic. Mainly do the >> null-check outside of the intrinsic for `Arrays.hashCode` cases. >> >> Having a centralized entry point means it'll be easier to parameterize the >> factor and start values which are now hard-coded (always 31, and a start >> value of either one for `Arrays` or zero for `String`). It seems somewhat >> premature to parameterize this up front. >> >> The current implementation is performance neutral on microbenchmarks on all >> tested platforms (x64, aarch64) when not enabling the intrinsic. We do add a >> few trivial method calls which increase the call stack depth, so surprises >> cannot be ruled out on complex workloads. >> >> With the most recent fixes the x64 intrinsic results on my workstation look >> like this: >> >> Benchmark (size) Mode Cnt ScoreError >> Units >> StringHashCode.Algorithm.defaultLatin1 1 avgt5 2.199 ± 0.017 >> ns/op >> StringHashCode.Algorithm.defaultLatin1 10 avgt5 6.933 ± 0.049 >> ns/op >> StringHashCode.Algorithm.defaultLatin1 100 avgt529.935 ± 0.221 >> ns/op >> StringHashCode.Algorithm.defaultLatin1 1 avgt5 1596.982 ± 7.020 >> ns/op >> >> Baseline: >> >> Benchmark (size) Mode Cnt ScoreError >> Units >> StringHashCode.Algorithm.defaultLatin1 1 avgt5 2.200 ± 0.013 >> ns/op >> StringHashCode.Algorithm.defaultLatin1 10 avgt5 9.424 ± 0.122 >> ns/op >> StringHashCode.Algorithm.defaultLatin1 100 avgt590.541 ± 0.512 >> ns/op >> StringHashCode.Algorithm.defaultLatin1 1 avgt5 9425.321 ± 67.630 >> ns/op >> >> I.e. no measurable overhead compared to baseline even for `size == 1`. >> >> The vectorized code now nominally works for all unsigned cases as well as >> ints, though more testing would be good. >> >> Benchmark for `Arrays.hashCode`: >> >> Benchmark (size) Mode Cnt ScoreError Units >> ArraysHashCode.bytes1 avgt5 1.884 ± 0.013 ns/op >> ArraysHashCode.bytes 10 avgt5 6.955 ± 0.040 ns/op >> ArraysHashCode.bytes 100 avgt587.218 ± 0.595 ns/op >> ArraysHashCode.bytes1 avgt5 9419.591 ± 38.308 ns/op >> ArraysHashCode.chars1 avgt5 2.200 ± 0.010 ns/op >> ArraysHashCode.chars 10 avgt5 6.935 ± 0.034 ns/op >> ArraysHashCode.chars 100 avgt530.216 ± 0.134 ns/op >> ArraysHashCode.chars1 avgt5 1601.629 ± 6.418 ns/op >> ArraysHashCode.ints 1 avgt5 2.200 ± 0.007 ns/op >> ArraysHashCode.ints10 avgt5 6.936 ± 0.034 ns/op >> ArraysHashCode.ints 100 avgt529.412 ± 0.268 ns/op >> ArraysHashCode.ints 1 avgt5 1610.578 ± 7.785 ns/op >> ArraysHashCode.shorts 1 avgt5 1.885 ± 0.012 ns/op >> ArraysHashCode.shorts 10 avgt5 6.961 ± 0.034 ns/op >> ArraysHashCode.shorts 100 avgt587.095 ± 0.417 ns/op >> ArraysHashCode.shorts 1 avgt5 9420.617 ± 50.089 ns/op >> >> Baseline: >> >> Benchmark (size) Mode Cnt ScoreError Units >> ArraysHashCode.bytes1 avgt5 3.213 ± 0.207 ns/op >> ArraysHashCode.bytes 10 avgt5 8.483 ± 0.040 ns/op >> ArraysHashCode.bytes 100 avgt590.315 ± 0.655 ns/op >> ArraysHashCode.bytes1 avgt5 9422.094 ± 62.402 ns/op >> ArraysHashCode.chars1 avgt5 3.040 ± 0.066 ns/op >> ArraysHashCode.chars 10 avgt5 8.497 ± 0.074 ns/op >> ArraysHashCode.chars 100 avgt590.074 ± 0.387 ns/op >> ArraysHashCode.chars1 avgt5 9420.474 ± 41.619 ns/op >> ArraysHashCode.ints 1 avgt5 2.827 ± 0.019 ns/op >> ArraysHashCode.ints10 avgt5 7.727 ± 0.043 ns/op >> ArraysHashCode.ints 100 avgt589.405 ± 0.593 ns/op >> ArraysHashCode.ints 1 avgt5 9426.539 ± 51.308 ns/op >> ArraysHashCode.shorts 1 avgt5 3.071 ± 0.062 ns/op >> ArraysHashCode.shorts 10 avgt5 8.168 ± 0.049 ns/op >> ArraysHashCode.shorts 100 avgt590.399 ± 0.292 ns/op >> ArraysHashCode.shorts 1 avgt5 9420.171 ± 44.474 ns/op >> >> >> As we can see the `Arrays` intrinsics are faster for small inputs, and >> faster on large inputs for `char` and `int` (the ones currently vectorized). >> I aim to fix `byte` and `short` cases before integrating, though it might be >> acceptable to hand that off as fo
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: 8297385: Remove duplicated null typos in javadoc
On Wed, 23 Nov 2022 06:58:09 GMT, Dongxu Wang wrote: > 8297385: Remove duplicated null typos in javadoc LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11311
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: 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: 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni [v2]
On Mon, 28 Nov 2022 10:02:47 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which fixes the typos in the test >> packages? @mernst submitted this as a separate PR >> https://github.com/openjdk/jdk/pull/10029 but given the number of areas and >> files that other PR touches, the progress has been stalled. >> >> I'll raise separate PRs in the other remaining areas from that other PR. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comment - don't change WithHelperPublisher.java Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11385
Re: RFR: 8296546: Add @spec tags to API [v3]
We hear you Mike :-) I have logged https://bugs.openjdk.org/browse/JDK-8297755 to update the links in java.net / java.net.http API documentation. best regards, -- daniel On 29/11/2022 03:14, Michael StJohns wrote: Hi - I need to repeat again. Please avoid using www.ietf.org as the URL base for referencing RFCs. The appropriate location is www.rfc-editor.org and is going to be more stable in the long run than any reference to an RFC that runs through the IETF's website. These two websites have different purposes, and the structure of the IETF website has changed at least once recently and may change again relatively (~5 years) soon. The most general and correct form for referencing RFCs is "https://www.rfc-editor.org/info/rfc" That will get you to the front page with pointers to all of the current semi-canonical versions of the spec (e.g. text, pdf-a, html, and xml). Mike
Re: RFR: 8297976: Remove sun.net.ProgressMonitor and related classes
On Fri, 2 Dec 2022 08:31:25 GMT, Daniel Jeliński wrote: > Please review this patch that removes progress monitoring classes used by > UrlConnection. > Since Java 9 these classes are not used in the JDK, and are not exported from > java.base. If anyone was still using them, reimplementing them in user code > should be pretty straightforward. > > This PR also fixes the issue where MeteredStream finalizer could resurrect an > unusable connection, causing unexpected exceptions in other parts of the code. > > No new regression test; since we are removing the finalizer, I don't believe > we will see the issue again. I can add a test for that if you think it still > makes sense. > > I had to adjust `ProxyModuleMapping.java` test which used > `sun.net.ProgressListener` as an example of a module-private interface; I > replaced it with another public interface from the same package. > > Existing tier 1-3 tests continue to pass. [JDK-8240275](https://bugs.openjdk.org/browse/JDK-8240275) seems to have a reproducer attached. Did you investigate whether turning that into a non-regression test would be worthwhile? Otherwise this looks like a good cleanup and simplification! - PR: https://git.openjdk.org/jdk/pull/11474
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: 8297976: Remove sun.net.ProgressMonitor and related classes [v2]
On Tue, 6 Dec 2022 12:07:35 GMT, Jaikiran Pai wrote: >> Daniel Jeliński has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add test > > test/jdk/sun/net/www/http/KeepAliveStream/KeepAliveStreamFinalizer.java line > 28: > >> 26: * @bug 8240275 >> 27: * @library /test/lib >> 28: * @run main/othervm KeepAliveStreamFinalizer > > Hello Daniel, it doesn't look like we are doing anything JVM specific in this > test. Is the `othervm` intentional here? The test calls `HttpsURLConnection.setDefaultSSLSocketFactor` which requires using `/othervm` - PR: https://git.openjdk.org/jdk/pull/11474
Re: RFR: 8297976: Remove sun.net.ProgressMonitor and related classes [v3]
On Mon, 5 Dec 2022 14:12:05 GMT, Daniel Jeliński wrote: >> Please review this patch that removes progress monitoring classes used by >> UrlConnection. >> Since Java 9 these classes are not used in the JDK, and are not exported >> from java.base. If anyone was still using them, reimplementing them in user >> code should be pretty straightforward. >> >> This PR also fixes the issue where MeteredStream finalizer could resurrect >> an unusable connection, causing unexpected exceptions in other parts of the >> code. >> >> No new regression test; since we are removing the finalizer, I don't believe >> we will see the issue again. I can add a test for that if you think it still >> makes sense. >> >> I had to adjust `ProxyModuleMapping.java` test which used >> `sun.net.ProgressListener` as an example of a module-private interface; I >> replaced it with another public interface from the same package. >> >> Existing tier 1-3 tests continue to pass. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Remove double space > > Co-authored-by: Andrey Turbanov LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11474
Re: RFR: 8297976: Remove sun.net.ProgressMonitor and related classes [v2]
On Tue, 6 Dec 2022 12:05:26 GMT, Jaikiran Pai wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add test
>
> test/jdk/sun/net/www/http/KeepAliveStream/KeepAliveStreamFinalizer.java line
> 175:
>
>> 173: public InputStream getInputStream() throws IOException {
>> 174: if (finalized) {
>> 175: System.out.println(failureReason = "getInputStream
>> called after finalize");
>
> For ease of debugging, perhaps use `System.err.println` instead of
> `System.out`? That way the `Thread.dumpStack()` which is done on the next
> line will appear in the same jtreg section as this message.
>
> Same comment in few other places where we have this construct.
I agree with Jaikiran that since we're not using testng here using `System.err`
consistently is probably better.
-
PR: https://git.openjdk.org/jdk/pull/11474
Re: RFR: 8297976: Remove sun.net.ProgressMonitor and related classes [v4]
On Tue, 6 Dec 2022 13:42:04 GMT, Daniel Jeliński wrote: >> Please review this patch that removes progress monitoring classes used by >> UrlConnection. >> Since Java 9 these classes are not used in the JDK, and are not exported >> from java.base. If anyone was still using them, reimplementing them in user >> code should be pretty straightforward. >> >> This PR also fixes the issue where MeteredStream finalizer could resurrect >> an unusable connection, causing unexpected exceptions in other parts of the >> code. >> >> No new regression test; since we are removing the finalizer, I don't believe >> we will see the issue again. I can add a test for that if you think it still >> makes sense. >> >> I had to adjust `ProxyModuleMapping.java` test which used >> `sun.net.ProgressListener` as an example of a module-private interface; I >> replaced it with another public interface from the same package. >> >> Existing tier 1-3 tests continue to pass. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Use System.err Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11474
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: [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: RFR: JDK-8300133: Use generalized see and link tags in core libs [v2]
On Fri, 13 Jan 2023 22:54:47 GMT, Joe Darcy wrote:
>> With generalized see and link tags that can refer to anchors (JDK-8200337),
>> the see and link tags in core libraries should be updated to use this
>> feature when possible. This PR covers such updates for java.base.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix typo found in code review.
src/java.base/share/classes/java/lang/CharSequence.java line 76:
> 74: *
> 75: * If the {@code char} value specified by the index is a
> 76: * {@linkplain Character##unicode surrogate}, the surrogate value
I didn't know about the `##` trick. Is that a new feature to target an HTML
anchor?
-
PR: https://git.openjdk.org/jdk/pull/12000
Re: RFR: JDK-8300133: Use generalized see and link tags in core libs [v2]
On Fri, 13 Jan 2023 22:54:47 GMT, Joe Darcy wrote: >> With generalized see and link tags that can refer to anchors (JDK-8200337), >> the see and link tags in core libraries should be updated to use this >> feature when possible. This PR covers such updates for java.base. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo found in code review. Changes to InetAddressResolverProvider.java look good. - PR: https://git.openjdk.org/jdk/pull/12000
Re: Integrated: 8301086: jdk/internal/util/ByteArray/ReadWriteValues.java fails with CompilationError
On Wed, 25 Jan 2023 14:02:14 GMT, Per Minborg wrote: > This PR suggests the removal of certain faulty tests. LGTM provided that the test passes with these changes. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/12196
Re: RFR: 8301367: Add exception handler method to the BaseLdapServer
On Tue, 31 Jan 2023 16:19:34 GMT, Aleksei Efimov wrote: > The proposed change adds a new exception handler method to the > `test/jdk/com/sun/jndi/ldap/lib/BaseLdapServer.java` LDAP test library class. > It will allow LDAP tests to customize the handling of server-side exceptions. > > The current `BaseLdapTestServer` implementation prints an exception and its > stack trace to the standard error stream. > > Existing tests in `test/jdk/com/sun/jndi/ldap` that use the modified library > class are passing with the modified version. Like Jaikiran I don't like the name of the new method much - but anything I could come up with was not much better. At least the new method gets both the socket and the exception as parameter, so that makes the name more reasonable. So approved as it stands! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/12347
Re: RFR: 8301737: java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java fail with -Xcomp
On Fri, 3 Feb 2023 03:52:39 GMT, SUN Guoyun wrote: > Hi all, > When -Xcomp be used, java thread to block for longer, then causing this test > failed frequently on the AArch64 and LoongArch64 architecture. > > This PR fix the issue, Please help review it. > > Thanks. test/jdk/java/rmi/server/UnicastRemoteObject/serialFilter/FilterUROTest.java line 41: > 39: /* > 40: * @test > 41: * @run testng/othervm -Dsun.net.httpserver.idleInterval=5 > FilterUROTest This test doesn't seem to be using an HttpServer so setting this system property seems useless as it should have no effect. - PR: https://git.openjdk.org/jdk/pull/12399
Re: RFR: 8298939: Refactor open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.sh to jtreg java test
On Mon, 9 Jan 2023 19:54:24 GMT, Matthew Donovan wrote: > Removed SSLSocketParametersTest.sh script (which just called a Java file) and > configured the java code to run directly with jtreg test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java line 192: > 190: throw exc; > 191: } > 192: } What is the story with the different exception catching here and in case 5? It doesn't look like a simple refactoring. Is another bug being fixed here? - PR: https://git.openjdk.org/jdk/pull/11910
Re: RFR: 8298939: Refactor open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.sh to jtreg java test
On Tue, 7 Feb 2023 16:02:30 GMT, Matthew Donovan wrote: >> test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java line 192: >> >>> 190: throw exc; >>> 191: } >>> 192: } >> >> What is the story with the different exception catching here and in case 5? >> It doesn't look like a simple refactoring. Is another bug being fixed here? > > The expected result of cases 4 and 5 is that an IllegalArgumentException is > thrown because of the unsupported ciphersuite. The original code just caught > `Exception` which would hide a problem in the test e.g., if > `SSLContext.getDefault()` throws the NoSuchAlgorithmException. OK - but the run( ) method catches IllegalArgumentException already? - PR: https://git.openjdk.org/jdk/pull/11910
Re: RFR: 8298939: Refactor open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.sh to jtreg java test [v2]
On Tue, 7 Feb 2023 19:33:23 GMT, Matthew Donovan wrote:
>> Removed SSLSocketParametersTest.sh script (which just called a Java file)
>> and configured the java code to run directly with jtreg
>
> Matthew Donovan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> clarified cases 4 and 5
test/jdk/javax/rmi/ssl/SSLSocketParametersTest.java line 184:
> 182: try {
> 183: new ServerFactory(SSLContext.getDefault(),
> 184: new String[]{"dummy_ciphersuite"}, null,
> false);
what if the constructor doesn't throw any exception? shouldn't the test fail?
or should `run` be called in that case?
-
PR: https://git.openjdk.org/jdk/pull/11910
Re: RFR: JDK-8301462: Convert Permission files to use lambda after JDK-8076596 [v2]
On Wed, 8 Feb 2023 03:42:16 GMT, Mandy Chung wrote: >> A trivial fix. Convert the use of anonymous inner classes in a few >> Permission classes to lambdas to work around JDK-8076596, which has been >> resolved. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > update copyright end year LGTM. I can't help noticing that the code for SocketPermission, PropertyPermission, and ServicePermission is almost identical. But there's not much we could do factorize that easily since the lambda requires access to private static method accessors: trying to factorize would probably add to the complexity instead of reducing it and impair readability. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/12463
Re: RFR: 8301627: System.exit and Runtime.exit debug logging
On Sun, 12 Feb 2023 18:15:25 GMT, Alan Bateman wrote:
>> It can be difficult to find the cause of calls to
>> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java
>> runtime exits.
>> The status value and stack trace are logged using the System Logger named
>> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.
>
> src/java.base/share/classes/java/lang/Shutdown.java line 162:
>
>> 160: * If the system logger {@code java.lang.Runtime} is enabled for
>> logging level DEBUG/FINE
>> 161: * the stack trace of the call to {@code Runtime.exit()} or {@code
>> System.exit()}
>> 162: * is logged.
>
> Shutdown is not a public class so this impNote won't appear in the APIs docs.
> Should it move to Runtime.exit and System.exit? If it moved to a public API
> then "system logger" could link to System.Logger. Also would it be more
> correct to say "a system logger named "java.lang.Runtime" is enabled for
> logging levels DEBUG or FINE"?
FINE is not a level supported by the System.Logger, it is the level to which
DEBUG is mapped when the backend is java.util.logging. I suggest to remove FINE
from this description and add an `{@link Loger.Level#DEBUG DEBUG}` around DEBUG.
> test/jdk/java/lang/runtime/RuntimeExitLogTest.java line 89:
>
>> 87: }
>> 88: cmd.add(this.getClass().getName());
>> 89: cmd.add(Integer.toString(status));
>
> Another possibility for testing this is to launch with ` --limit-modules
> java.base -Djdk.system.logger.level=DEBUG` to use the simple console
> implementation that is in java.base and avoid needing properties files for
> j.u.logging. Just mentioning is an option to make it simple.
Good point - though maybe both configurations should be tested. The
j.u.l.LogManager registers some shutdown hook so I believe it's important to
test with j.u.l present too.
-
PR: https://git.openjdk.org/jdk/pull/12517
Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v2]
On Tue, 14 Feb 2023 07:45:07 GMT, Alan Bateman wrote:
>> FINE is not a level supported by the System.Logger, it is the level to which
>> DEBUG is mapped when the backend is java.util.logging. I suggest to remove
>> FINE from this description and add an `{@link Loger.Level#DEBUG DEBUG}`
>> around DEBUG.
>
> Roger has updated this but it's still a comment on a non-public class. I
> think the main question here is whether there should be a note in the
> System.exit and Runtime.exit to document that these methods log? If not, will
> it be documented anywhere, maybe a troubleshooting guide to help track down
> what/who is calling System.exit?
Another way to document that could be to add the commented logger name, and a
comment, to
https://github.com/openjdk/jdk/blob/master/src/java.logging/share/conf/logging.properties
A new section at the end with some explanation on what this logger prints and
how to enable it - with the appropriate wording to make it clear that it's
JDK-implementation specific and not part of the spec.
-
PR: https://git.openjdk.org/jdk/pull/12517
Re: RFR: 8301627: System.exit and Runtime.exit debug logging [v3]
On Tue, 14 Feb 2023 16:46:29 GMT, Roger Riggs wrote:
>> It can be difficult to find the cause of calls to
>> `java.lang.System.exit(status)` and `Runtime.exit(status)` because the Java
>> runtime exits.
>> The status value and stack trace are logged using the System Logger named
>> `java.lang.Runtime` with message level `System.Logger.Level.DEBUG`.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add an @implNote to Runtime.exit to describe the java.lang.Runtime logging.
src/java.base/share/classes/java/lang/Runtime.java line 160:
> 158: *
> 159: * @implNote
> 160: * If the {@link System.Logger#getLogger(String)
> System.Logger.getLogger("java.lang.Runtime")}
The link looks wrong to me:
Suggestion:
* If the {@link System#getLogger(String)
System.getLogger("java.lang.Runtime")}
See
https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/System.html#getLogger(java.lang.String)
-
PR: https://git.openjdk.org/jdk/pull/12517
Re: RFR: 8302659: Modernize Windows native code for NetworkInterface
On Thu, 16 Feb 2023 14:32:15 GMT, Rich DiCroce wrote: > Improves performance and correctness, as discussed on the net-dev mailing > list. FWIW - there is a perl script located in `make/scripts/normalizer.pl` that can be run on modified sources to fix up whitespace and CRLF issues when jcheck complains. - PR: https://git.openjdk.org/jdk/pull/12593
Re: RFR: 8302664: Fix several incorrect usages of Preconditions.checkFromIndexSize
On Thu, 16 Feb 2023 14:42:52 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which fixes the usage of > `Preconditions.checkFromIndexSize`? This addresses > https://bugs.openjdk.org/browse/JDK-8302664. > > There was an oversight when these changes were introduced in > https://github.com/openjdk/jdk/pull/4507. I have now gone through that patch > again to make sure the relevant places where this fix is needed has been > addressed in this current PR. > > I have also looked into other changes in that PR, just to be sure that there > aren't any similar fixes needed for other method calls that were introduced > in it - they all look fine. > > tier1,tier2 and tier3 testing with this change passed successfully. I thought > (and experimented a bit) to add new tests for Deflater/Inflater to catch > these byte array indexing issues, but it wasn't straight forward and I would > have to write the entire inflate/deflate test just to verify this issue. So I > decided to leave out new tests for now in this PR. As Daniel noted the computation is symmetrical which explains why it worked. But passing inverted parameters was confusing. LGTM2. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/12595
Re: RFR: 8298939: Refactor open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.sh to jtreg java test [v3]
On Tue, 7 Feb 2023 20:38:40 GMT, Matthew Donovan wrote: >> Removed SSLSocketParametersTest.sh script (which just called a Java file) >> and configured the java code to run directly with jtreg > > Matthew Donovan has updated the pull request incrementally with one > additional commit since the last revision: > > added exceptions for cases 4 and 5 Thanks for the update. I believe this looks good, but it would be nice to get @stuart-marks or @RogerRiggs approval before integrating. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11910
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v3]
On Wed, 22 Feb 2023 16:17:11 GMT, Alan Bateman wrote:
>> Executors.newSingleThreadExecutor returns a delegating ExecutorService that
>> has finalizer to shutdown the underlying TPE when the wrapper is
>> finalizable. It goes back to JDK 6 and JDK-6399443. This is the last
>> non-empty finalizer in java.base. Removing it will likely lead to bug
>> reports/complaints as the current behavior goes back to 2006. So the
>> proposal is to just replace it with a Cleaner, trivially done in this case.
>> As part of the changes, I've replaced the existing test with a more modern
>> test that exercises more scenarios.
>
> Alan Bateman 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:
>
> - Keep reference to Cleanable
> - Merge
> - Fix typo in comment, remove blank line
> - Replace older test
> - Initial commit
src/java.base/share/classes/java/util/concurrent/Executors.java line 844:
> 842: @Override
> 843: public void shutdown() {
> 844: cleaner.clean();
Hmmm... so now shutdown no longer requires permission. You should probably call
super.shutdown() before invoking cleaner.clean(), assuming that shutdown() can
be called twice.
-
PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v4]
On Thu, 23 Feb 2023 08:31:36 GMT, Alan Bateman wrote: >> Executors.newSingleThreadExecutor returns a delegating ExecutorService that >> has finalizer to shutdown the underlying TPE when the wrapper is >> finalizable. It goes back to JDK 6 and JDK-6399443. This is the last >> non-empty finalizer in java.base. Removing it will likely lead to bug >> reports/complaints as the current behavior goes back to 2006. So the >> proposal is to just replace it with a Cleaner, trivially done in this case. >> As part of the changes, I've replaced the existing test with a more modern >> test that exercises more scenarios. > > Alan Bateman 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 seven additional > commits since the last revision: > > - Merge > - Improve SM scenario > - Keep reference to Cleanable > - Merge > - Fix typo in comment, remove blank line > - Replace older test > - Initial commit Given that the delegated executor can only be an instance of ThreadPoolExecutor then these new changes LGTM. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/12675
Re: RFR: 8302899: Executors.newSingleThreadExecutor can use Cleaner to shutdown executor [v4]
On Thu, 23 Feb 2023 08:31:36 GMT, Alan Bateman wrote: >> Executors.newSingleThreadExecutor returns a delegating ExecutorService that >> has finalizer to shutdown the underlying TPE when the wrapper is >> finalizable. It goes back to JDK 6 and JDK-6399443. This is the last >> non-empty finalizer in java.base. Removing it will likely lead to bug >> reports/complaints as the current behavior goes back to 2006. So the >> proposal is to just replace it with a Cleaner, trivially done in this case. >> As part of the changes, I've replaced the existing test with a more modern >> test that exercises more scenarios. > > Alan Bateman 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 seven additional > commits since the last revision: > > - Merge > - Improve SM scenario > - Keep reference to Cleanable > - Merge > - Fix typo in comment, remove blank line > - Replace older test > - Initial commit src/java.base/share/classes/java/util/concurrent/Executors.java line 830: > 828: * ExecutorService when the wrapper becomes phantom reachable. > 829: */ > 830: private static class AutoShutdownDelegatedExecutorService nit: if it's not extended anywhere maybe it could be marked final as well. - PR: https://git.openjdk.org/jdk/pull/12675
