Re: Accessing 'system' header fields implicitly added by HttpClient
Hi Moataz, This sounds like a reasonable request, with a concrete use case. I have logged https://bugs.openjdk.java.net/browse/JDK-8267510 best regards, -- daniel On 20/05/2021 15:58, Moataz Abdelnasser wrote: Hi! I've recently released an HTTP cache (https://mizosoft.github.io/methanol/caching) that operates as part of an interceptor chain built atop a standard HttpClient. Currently, the cache can store only one response per request. However, request headers can specify multiple response variants if nominated by the 'Vary' header (e.g. gzip & deflate variants if varying on Accept-Encoding). To ensure a response is selected only if it is the corrected variant, incoming requests' Vary-nominated fields must match against those in the initiating request. The bummer is that HttpClient can augment requests with implicit header fields (e.g. Cookie, Authorization, etc.), and accessing these seems to be impossible (tested that with 'response.request().headers()', please correct me if I'm wrong). The cache has to render a response uncacheable if it decides it varies on these fields, as their values cannot be known in the initiating request, so they can't be matched against when new requests are made. It'd be nice if one could access these headers, maybe via 'response.request().headers()', or something like 'response.networkRequest().headers()'. This would increase cache's efficiency. Additionally, I can find it useful for debugging as it's always good to know what modifications the client makes to requests. Regards, Moataz
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
typo on windows
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4138/files
- new: https://git.openjdk.java.net/jdk/pull/4138/files/e3f0405a..d460efb8
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов wrote: > The usage of `LinkedList` is senseless and can be replaced with either > `ArrayList` or `ArrayDeque` which are both more compact and effective. > > jdk:tier1 and jdk:tier2 are both ok Hi guys, any more comments here? Please ping me if I've missed something - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 13:13:04 GMT, Сергей Цыпанов wrote: > Hi guys, any more comments here? Please ping me if I've missed something I suspect this will require renaming some fields and changing comments, e.g. requestList is no longer a good name for the field in AbstractPoller, its comments need changes too. The field in ResolverConfigurationImpl is searchList, it will require a few changes. There may be more, I just picked out a few. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов
wrote:
> The usage of `LinkedList` is senseless and can be replaced with either
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
>
> jdk:tier1 and jdk:tier2 are both ok
I don't remember all the comments you have received in this thread but have you
verified that arbitrarily changing `LinkedList` into `ArrayList` in these
classes is not going to significantly increase the footprint in the case where
lists are empty or contain only one or two elements?
I am not convinced that a global replacement of `LinkedList` by `ArrayList` is
necessarily good - even though I agree that `ArrayList` is generally more
efficient.
src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 155:
> 153: */
> 154: public List get(String fileName) {
> 155: ArrayList jarFiles;
This could probably be declared as:
List jarFiles;
src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 264:
> 262: String jar = jarFiles[i];
> 263: bw.write(jar + "\n");
> 264: ArrayList jarlist = jarMap.get(jar);
Here again, jarList could probably be declared as `List`
-
PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 26 Feb 2021 10:48:33 GMT, Сергей Цыпанов
wrote:
> The usage of `LinkedList` is senseless and can be replaced with either
> `ArrayList` or `ArrayDeque` which are both more compact and effective.
>
> jdk:tier1 and jdk:tier2 are both ok
src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 154:
> 152: * @param fileName the key of the mapping
> 153: */
> 154: public List get(String fileName) {
IcedTea-Web seems to be using this method reflectively:
https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80
-
PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 14:52:21 GMT, Thiago Henrique Hüpner wrote: > IcedTea-Web seems to be using this method reflectively: > https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80 I assume this doesn't work with JDK 16, at least not without using --add-options to export jdk.internal.util.jar. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 15:00:15 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/jdk/internal/util/jar/JarIndex.java line 154:
>>
>>> 152: * @param fileName the key of the mapping
>>> 153: */
>>> 154: public List get(String fileName) {
>>
>> IcedTea-Web seems to be using this method reflectively:
>> https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80
>
>> IcedTea-Web seems to be using this method reflectively:
>> https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/common/src/main/java/net/adoptopenjdk/icedteaweb/jdk89access/JarIndexAccess.java#L80
>
> I assume this doesn't work with JDK 16, at least not without using
> --add-options to export jdk.internal.util.jar.
Just for completeness, they're using the add-exports:
https://github.com/AdoptOpenJDK/IcedTea-Web/blob/master/launchers/itw-modularjdk.args#L19
-
PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
src/java.base/share/classes/java/lang/SecurityManager.java line 104:
> 102: * method will throw an {@code UnsupportedOperationException}). If the
> 103: * {@systemProperty java.security.manager} system property is set to the
> 104: * special token "{@code allow}", then a security manager will not be
> set at
Can/should the `{@systemProperty ...}` tag be used more than once for a given
system property? I thought it should be used only once, at the place where the
system property is defined. Maybe @jonathan-gibbons can offer some more
guidance on this.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 14:00:39 GMT, Weijun Wang wrote:
>> The code change refactors classes that have a `SuppressWarnings("removal")`
>> annotation that covers more than 50KB of code. The big annotation is often
>> quite faraway from the actual deprecated API usage it is suppressing, and
>> with the annotation covering too big a portion it's easy to call other
>> deprecated methods without being noticed.
>>
>> The code change shows some common solutions to avoid such too wide
>> annotations:
>>
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local
>> variable and add annotation on the declaration, and then assign that value
>> to the original l-value.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> typo on windows
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
> 118: vals[1] =
> Integer.getInteger("sun.net.client.defaultConnectTimeout",
> 300_000).intValue();
> 119: return System.getProperty("file.encoding",
> "ISO8859_1");
> 120: }
It is a bit strange that "file.encoding" seem to get a special treatment - but
I guess that's OK.
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
> 548: * @throws IOException if the connection was unsuccessful.
> 549: */
> 550: @SuppressWarnings("removal")
Could the scope of the annotation be further reduced by applying it to the two
places where `doPrivileged` is called in this method?
src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 921:
> 919: }
> 920:
> 921: @SuppressWarnings("removal")
Could the scope be further reduced by applying it at line 926 in this method -
at the cost of creating a temporary variable? The code could probably be
further simplified by using a lambda...
PrivilegedAction pa = () -> new Socket(proxy);
@SuppressWarnings("removal")
var ps = AccessController.doPrivileged(pa);
s = ps;
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: JDK-8265362 java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
On Tue, 18 May 2021 22:43:14 GMT, Mark Sheppard wrote:
> The test java/net/Socket/UdpSocket.java has been seen to fail with a
> BindException, in the testMaxSockets test, on a regular basis on
> macOS-aarch64 platform. testMaxSockets tests the maximum number of UDP
> Sockets that may be created as defined by a system property
> sun.net.maxDatagramSockets. It invokes the Socket constructor
> Socket(InetAddress host, int port, boolean stream) with stream set to false
> to create a UDP Socket. This instantiation is a compound operation,
> consisting of the creation of a socket, the explicit binding of wildcard
> address and ephemeral port, and a connect to the socket end point specified
> in the constructor parameters. Analysis has shown that during the test that
> the OS intermittently allocates the same ephemeral port multiple times during
> the bind system call, such that two separate sockets end up bound to the same
> port. Then on the connect invocation a BindException is thrown for the second
> socket. This has been determined to be a transient condition duri
ng heavy loads and where a significant number of ephemeral ports are being
allocated.
>
> As this is deemed an OS issues, and in order to increase test stability, it
> has been found that for the BindException condition a retry of the Socket
> creation mitigates the issues and tests the max creation property.
test/jdk/java/net/Socket/UdpSocket.java line 151:
> 149: }
> 150: }
> 151: return newUdpSocket;
I added this test in advance of JEP 353 as we didn't have much coverage for
this deprecated constructor. No objection to the retry if it helps. Is the
catching of SocketException a left over from testing? I assume the patch can be
simplified down to:
try {
return new Socket(InetAddress.getLoopbackAddress(), 8000, false);
} catch (BindException e) {
System.out.println(...);
return new Socket(InetAddress.getLoopbackAddress(), 8000, false);
}
-
PR: https://git.openjdk.java.net/jdk/pull/4103
Re: RFR: JDK-8265362 java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
On Wed, 19 May 2021 05:56:07 GMT, Alan Bateman wrote:
>> The test java/net/Socket/UdpSocket.java has been seen to fail with a
>> BindException, in the testMaxSockets test, on a regular basis on
>> macOS-aarch64 platform. testMaxSockets tests the maximum number of UDP
>> Sockets that may be created as defined by a system property
>> sun.net.maxDatagramSockets. It invokes the Socket constructor
>> Socket(InetAddress host, int port, boolean stream) with stream set to false
>> to create a UDP Socket. This instantiation is a compound operation,
>> consisting of the creation of a socket, the explicit binding of wildcard
>> address and ephemeral port, and a connect to the socket end point specified
>> in the constructor parameters. Analysis has shown that during the test that
>> the OS intermittently allocates the same ephemeral port multiple times
>> during the bind system call, such that two separate sockets end up bound to
>> the same port. Then on the connect invocation a BindException is thrown for
>> the second socket. This has been determined to be a transient condition dur
ing heavy loads and where a significant number of ephemeral ports are being
allocated.
>>
>> As this is deemed an OS issues, and in order to increase test stability, it
>> has been found that for the BindException condition a retry of the Socket
>> creation mitigates the issues and tests the max creation property.
>
> test/jdk/java/net/Socket/UdpSocket.java line 151:
>
>> 149: }
>> 150: }
>> 151: return newUdpSocket;
>
> I added this test in advance of JEP 353 as we didn't have much coverage for
> this deprecated constructor. No objection to the retry if it helps. Is the
> catching of SocketException a left over from testing? I assume the patch can
> be simplified down to:
>
>
> try {
>return new Socket(InetAddress.getLoopbackAddress(), 8000, false);
> } catch (BindException e) {
> System.out.println(...);
>return new Socket(InetAddress.getLoopbackAddress(), 8000, false);
> }
yes, thanks for that ... updated as requested
-
PR: https://git.openjdk.java.net/jdk/pull/4103
RFR: JDK-8265362 java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
The test java/net/Socket/UdpSocket.java has been seen to fail with a BindException, in the testMaxSockets test, on a regular basis on macOS-aarch64 platform. testMaxSockets tests the maximum number of UDP Sockets that may be created as defined by a system property sun.net.maxDatagramSockets. It invokes the Socket constructor Socket(InetAddress host, int port, boolean stream) with stream set to false to create a UDP Socket. This instantiation is a compound operation, consisting of the creation of a socket, the explicit binding of wildcard address and ephemeral port, and a connect to the socket end point specified in the constructor parameters. Analysis has shown that during the test that the OS intermittently allocates the same ephemeral port multiple times during the bind system call, such that two separate sockets end up bound to the same port. Then on the connect invocation a BindException is thrown for the second socket. This has been determined to be a transient condition during heavy loads and where a significant number of ephemeral ports are being allocated. As this is deemed an OS issues, and in order to increase test stability, it has been found that for the BindException condition a retry of the Socket creation mitigates the issues and tests the max creation property. - Commit messages: - JDK-8265362 java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64) - JDK-8265362: additions to execute a retry of UDP Socket construction if a BindException thrown during the testMaxSockets test Changes: https://git.openjdk.java.net/jdk/pull/4103/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4103&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265362 Stats: 14 lines in 2 files changed: 10 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4103.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4103/head:pull/4103 PR: https://git.openjdk.java.net/jdk/pull/4103
Re: RFR: JDK-8265362 java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
On Tue, 18 May 2021 22:43:14 GMT, Mark Sheppard wrote: > The test java/net/Socket/UdpSocket.java has been seen to fail with a > BindException, in the testMaxSockets test, on a regular basis on > macOS-aarch64 platform. testMaxSockets tests the maximum number of UDP > Sockets that may be created as defined by a system property > sun.net.maxDatagramSockets. It invokes the Socket constructor > Socket(InetAddress host, int port, boolean stream) with stream set to false > to create a UDP Socket. This instantiation is a compound operation, > consisting of the creation of a socket, the explicit binding of wildcard > address and ephemeral port, and a connect to the socket end point specified > in the constructor parameters. Analysis has shown that during the test that > the OS intermittently allocates the same ephemeral port multiple times during > the bind system call, such that two separate sockets end up bound to the same > port. Then on the connect invocation a BindException is thrown for the second > socket. This has been determined to be a transient condition duri ng heavy loads and where a significant number of ephemeral ports are being allocated. > > As this is deemed an OS issues, and in order to increase test stability, it > has been found that for the BindException condition a retry of the Socket > creation mitigates the issues and tests the max creation property. Thanks for taking this on Mark! The proposed changes look good to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4103
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 07:06:00 GMT, Alan Bateman wrote: >> The JEP isn't PTT for 17 so there's plenty of time isn't there ? > > There are 827 files in this patch. Phil is right that adding SW at the class > level is introducing technical debt but if addressing that requires > refactoring and re-testing of AWT or font code then it would be better to > have someone from that area working on. Is there any reason why this can't be > going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I > don't think we should put Max through the wringer here as there are too many > areas to cover. Are you suggesting that the patch doesn't need testing as it is ? It should be the same either way. It is very straight forward to run the automated tests across all platforms these days. I get the impression that no one is guaranteeing to do this straight after integration. It sounds like it is up for deferral if time runs out. The amount of follow-on work that I am hearing about here, and may be for tests, does not make it sound like this JEP is nearly as done as first presented. If there was some expectation that groups responsible for an area would get involved with this issue which I am assured was already known about, then why was it not raised before and made part of the plan ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Tue, 18 May 2021 21:44:43 GMT, Weijun Wang wrote:
>> Please review the test changes for [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> With JEP 411 and the default value of `-Djava.security.manager` becoming
>> `disallow`, tests calling `System.setSecurityManager()` need
>> `-Djava.security.manager=allow` when launched. This PR covers such changes
>> for tier1 to tier3 (except for the JCK tests).
>>
>> To make it easier to focus your review on the tests in your area, this PR is
>> divided into multiple commits for different areas (~~serviceability~~,
>> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
>> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
>> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is
>> the same as how Skara adds labels, but there are several small tweaks:
>>
>> 1. When a file is covered by multiple labels, only one is chosen. I make my
>> best to avoid putting too many tests into `core-libs`. If a file is covered
>> by `core-libs` and another label, I categorized it into the other label.
>> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
>> `xml` commit.
>> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
>> `rmi` commit.
>> 4. One file not covered by any label --
>> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
>> the `swing` commit.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> all files to minimize unnecessary merge conflict.
>>
>> Please note that this PR can be integrated before the source changes for JEP
>> 411, as the possible values of this system property was already defined long
>> time ago in JDK 9.
>>
>> Most of the change in this PR is a simple adding of
>> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes
>> it was not `othervm` and we add one. Sometimes there's no `@run` at all and
>> we add the line.
>>
>> There are several tests that launch another Java process that needs to call
>> the `System.setSecurityManager()` method, and the system property is added
>> to `ProcessBuilder`, `ProcessTools`, or the java command line (if the test
>> is a shell test).
>>
>> 3 langtools tests are added into problem list due to
>> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>>
>> 2 SQL tests are moved because they need different options on the `@run` line
>> but they are inside a directory that has a `TEST.properties`:
>>
>> rename test/jdk/java/sql/{testng/test/sql/othervm =>
>> permissionTests}/DriverManagerPermissionsTests.java (93%)
>> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
>> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>> ```
>>
>> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> adjust order of VM options
The client changes are fine except for the one misplaced (c)
test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:
> 1: /*
> 2: * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights
> reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
Probably the (c) update was meant to be on the .sh file that is actually
modified.
-
Marked as reviewed by prr (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]
On Fri, 21 May 2021 18:26:48 GMT, Phil Race wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> adjust order of VM options > > test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3: > >> 1: /* >> 2: * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > Probably the (c) update was meant to be on the .sh file that is actually > modified. Oops, I'll back it out. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v3]
> Please review the test changes for [JEP
> 411](https://openjdk.java.net/jeps/411).
>
> With JEP 411 and the default value of `-Djava.security.manager` becoming
> `disallow`, tests calling `System.setSecurityManager()` need
> `-Djava.security.manager=allow` when launched. This PR covers such changes
> for tier1 to tier3 (except for the JCK tests).
>
> To make it easier to focus your review on the tests in your area, this PR is
> divided into multiple commits for different areas (~~serviceability~~,
> ~~hotspot-compiler~~, ~~i18n~~, ~~rmi~~, ~~javadoc~~, swing, 2d,
> ~~security~~, ~~hotspot-runtime~~, ~~nio~~, ~~xml~~, ~~beans~~,
> ~~core-libs~~, ~~net~~, ~~compiler~~, ~~hotspot-gc~~). Mostly the rule is the
> same as how Skara adds labels, but there are several small tweaks:
>
> 1. When a file is covered by multiple labels, only one is chosen. I make my
> best to avoid putting too many tests into `core-libs`. If a file is covered
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the
> `rmi` commit.
> 4. One file not covered by any label --
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in
> the `swing` commit.
>
> Due to the size of this PR, no attempt is made to update copyright years for
> all files to minimize unnecessary merge conflict.
>
> Please note that this PR can be integrated before the source changes for JEP
> 411, as the possible values of this system property was already defined long
> time ago in JDK 9.
>
> Most of the change in this PR is a simple adding of
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we
> add the line.
>
> There are several tests that launch another Java process that needs to call
> the `System.setSecurityManager()` method, and the system property is added to
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a
> shell test).
>
> 3 langtools tests are added into problem list due to
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
>
> 2 SQL tests are moved because they need different options on the `@run` line
> but they are inside a directory that has a `TEST.properties`:
>
> rename test/jdk/java/sql/{testng/test/sql/othervm =>
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi =>
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
> ```
>
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
feedback from Phil
reverted:
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4071/files
- new: https://git.openjdk.java.net/jdk/pull/4071/files/bfa955ad..9a3ec578
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
PR: https://git.openjdk.java.net/jdk/pull/4071
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:35:05 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
>
>> 118: vals[1] =
>> Integer.getInteger("sun.net.client.defaultConnectTimeout",
>> 300_000).intValue();
>> 119: return System.getProperty("file.encoding",
>> "ISO8859_1");
>> 120: }
>
> It is a bit strange that "file.encoding" seem to get a special treatment -
> but I guess that's OK.
You might say we thus avoid wasting the return value, as much as possible.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]
On Fri, 21 May 2021 15:37:48 GMT, Daniel Fuchs wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
>
>> 548: * @throws IOException if the connection was unsuccessful.
>> 549: */
>> 550: @SuppressWarnings("removal")
>
> Could the scope of the annotation be further reduced by applying it to the
> two places where `doPrivileged` is called in this method?
I'll probably need to assign the doPriv result on L631 to a tmp variable and
then assign it to `s`. Are you OK with this ugliness? Update: Ah, I see you
already have similar suggestion in the next comment.
-
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]
> The code change refactors classes that have a `SuppressWarnings("removal")`
> annotation that covers more than 50KB of code. The big annotation is often
> quite faraway from the actual deprecated API usage it is suppressing, and
> with the annotation covering too big a portion it's easy to call other
> deprecated methods without being noticed.
>
> The code change shows some common solutions to avoid such too wide
> annotations:
>
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local
> variable and add annotation on the declaration, and then assign that value to
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
update FtpClient.java
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4138/files
- new: https://git.openjdk.java.net/jdk/pull/4138/files/d460efb8..60d97a4c
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=01-02
Stats: 23 lines in 1 file changed: 0 ins; 12 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138
PR: https://git.openjdk.java.net/jdk/pull/4138
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
I haven't seen any response to this comment I made a couple of days ago and I
suspect it got missed
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {
Who or what prompted this change ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Fri, 21 May 2021 20:43:05 GMT, Phil Race wrote:
> I haven't seen any response to this comment I made a couple of days ago and I
> suspect it got missed
>
> > Weijun Wang has updated the pull request incrementally with one additional
> > commit since the last revision:
> > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
>
> > 57: ProcessCommunicator
> > 58: .executeChildProcess(Consumer.class, new
> > String[0]);
> > 59: if (!"Hello".equals(processResults.getStdOut())) {
>
> Who or what prompted this change ?
I replied right in the thread but unfortunately GitHub does not display it at
the end of page.
This is because the process is now launched with
`-Djava.security.manager=allow` (because of another change in
https://github.com/openjdk/jdk/pull/4071), and a new warning is displayed in
stderr. Therefore I switched to stdout.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8263561: Re-examine uses of LinkedList
On Fri, 21 May 2021 14:22:47 GMT, Daniel Fuchs wrote: >> The usage of `LinkedList` is senseless and can be replaced with either >> `ArrayList` or `ArrayDeque` which are both more compact and effective. >> >> jdk:tier1 and jdk:tier2 are both ok > > I don't remember all the comments you have received in this thread but have > you verified that arbitrarily changing `LinkedList` into `ArrayList` in these > classes is not going to significantly increase the footprint in the case > where lists are empty or contain only one or two elements? > > I am not convinced that a global replacement of `LinkedList` by `ArrayList` > is necessarily good - even though I agree that `ArrayList` is generally more > efficient. I second the footprint concerns from @dfuch. I've seen with first hand cases where widespread uses of array lists for holding 1-2-3 elements (e.g. think of a graph where each node might only have a very small number of outgoing edges) lead to massive memory over-utilization. If the average size is known, at the very least I'd argue to replace with an array list which is sized correctly. And, all this said, the general assumption implied in this PR which linked lists are just to be avoided doesn't match my experience. If you want a "pay only as much memory as you use" data structure, you don't care about random access (e.g. all accesses are linear walks), a linked list is a perfectly fine choice. If there are use cases in the JDK where a LinkedList is used in places where it shouldn't be, then obviously _those cases_ should be replaced. - PR: https://git.openjdk.java.net/jdk/pull/2744
Re: RFR: JDK-8265362 java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
On Fri, 21 May 2021 17:00:11 GMT, Mark Sheppard wrote:
>> test/jdk/java/net/Socket/UdpSocket.java line 151:
>>
>>> 149: }
>>> 150: }
>>> 151: return newUdpSocket;
>>
>> I added this test in advance of JEP 353 as we didn't have much coverage for
>> this deprecated constructor. No objection to the retry if it helps. Is the
>> catching of SocketException a left over from testing? I assume the patch can
>> be simplified down to:
>>
>>
>> try {
>>return new Socket(InetAddress.getLoopbackAddress(), 8000, false);
>> } catch (BindException e) {
>> System.out.println(...);
>>return new Socket(InetAddress.getLoopbackAddress(), 8000, false);
>> }
>
> yes, thanks for that ... updated as requested
Thanks, and if you want to keep it consistent with the existing code then you
could rename "Socket newUdpSocket" and "biEx", or just change it to "return new
Socket(...)".
-
PR: https://git.openjdk.java.net/jdk/pull/4103
Re: RFR: JDK-8265362 java/net/Socket/UdpSocket.java fails with "java.net.BindException: Address already in use" (macos-aarch64)
On Sat, 22 May 2021 06:45:33 GMT, Alan Bateman wrote: >> yes, thanks for that ... updated as requested > > Thanks, and if you want to keep it consistent with the existing code then you > could rename "Socket newUdpSocket" and "biEx", or just change it to "return > new Socket(...)". BTW: Is one retry enough? There is at least one other replace where we've had to retry to workaround a macOS bug and one retry was enough there too. - PR: https://git.openjdk.java.net/jdk/pull/4103
