Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]
On Tue, 23 Feb 2021 23:32:01 GMT, Stuart Marks wrote: >>> > Is there any behavior change here that merits a CSR review? >>> >>> Maybe. The one observable change is that calling `Collections.bar(foo)` >>> with a `foo` that is already a `bar` will return the instance rather than >>> unnecessarily wrap it. This could change semantics in applications >>> inadvertently or deliberately relying on identity. >> >> Yes. The CSR was to consider primarily this case. Probably out of an >> abundance of caution here. @stuart-marks may have another case to consider. > >> Is there any behavior change here that merits a CSR review? > > Yes. See my comments in the bug report: > > https://bugs.openjdk.java.net/browse/JDK-6323374?focusedCommentId=14296330=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14296330 > > There is not only the issue of the identity of the object returned, but the > change is also observable in the serialized form. Most people would consider > the change (less nesting) to be an improvement, but the change is observable, > and as we know any observable behavior can become depended upon by > applications. Code changes all look good. I'm thinking that we should add `@implNote` clauses to all the docs of the affected methods, saying something like "This method may return its argument if it is already unmodifiable." Usually it's reasonable to leave these kinds of behaviors unspecified (and we do so elsewhere) but since this is a change in long-standing behavior, it seems reasonable to highlight it explicitly. I don't think we want to specify it though, because of the issue with ImmutableCollections (as discussed previously) and possible future tuning of behavior regarding the various Set and Map subinterfaces. (For example, C.unmodifiableSet(arg) could return arg if it's an UnmodifiableNavigableSet.) The test seems to have a lot of uncomfortable dependencies, both explicit and implicit, on the various ImmutableCollection and UnmodifiableX implementation classes. Would it be sufficient to test various instances for reference equality and inequality instead? For example, something like var list0 = List.of(); var list1 = Collections.unmodifiableList(list0); var list2 = Collections.unmodifiableList(list1); assertNotSame(list0, list1); assertSame(list1, list2); This would avoid having to write test cases that cover various internal classes. The ImmutableCollections classes have been reorganized in the past, and while we don't have any plans to do so again at the moment, there is always the possibility of it happening again. One could write out all the different cases "by hand" but there are rather a lot of them. It might be fruitful to extract the "wrap once, wrap again, assertNotSame, assertSame" logic into a generic test and drive it somehow with a data provider that provides the base instance and a wrapper function. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]
On Tue, 23 Feb 2021 16:27:06 GMT, Ian Graves wrote: > Is there any behavior change here that merits a CSR review? Yes. See my comments in the bug report: https://bugs.openjdk.java.net/browse/JDK-6323374?focusedCommentId=14296330=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14296330 There is not only the issue of the identity of the object returned, but the change is also observable in the serialized form. Most people would consider the change (less nesting) to be an improvement, but the change is observable, and as we know any observable behavior can become depended upon by applications. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]
On Fri, 19 Feb 2021 01:52:51 GMT, liach
wrote:
>> Maybe it is not correct for UnmodifiableEntrySet::contains to short circuit?
>> What if the implementation was changed to:
>>
>> `public boolean contains(Object o) {
>> if (!(o instanceof Map.Entry))
>> return c.contains(o); //false, NPE, or CCE
>> return c.contains(
>> new UnmodifiableEntry<>((Map.Entry) o));
>> }`
>
> This however changes the behavior of unmodifiable maps compared to before;
> i.e. before for other entry sets, they could not throw exception if the
> object passed was not map entry; now they can.
Yes, it seems reasonable to decouple the `ImmutableCollections` issues from
this change.
-
PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides [v2]
On Tue, 23 Feb 2021 22:12:38 GMT, Daniel Fuchs wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8258444: Explcitly inherit IOOBE in {Filter,InputStream}Reader
>
> src/java.base/share/classes/java/io/PushbackReader.java line 98:
>
>> 96: * {@inheritDoc}
>> 97: */
>> 98: public int read(char[] cbuf, int off, int len) throws IOException {
>
> Shouldn't you add
>
> * @throws IndexOutOfBoundException {@inheritDoc}
> * @throws IOException {@inheritDoc}
>
> here as well? IIRC the global {@inheritDoc} will not add them.
No. In this case the specification no longer appears in the main method summary
but rather under `Methods declared in class java.io.FilterReader` which has a
full spec.
-
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides [v2]
On Tue, 23 Feb 2021 17:42:58 GMT, Brian Burkhalter wrote:
>> 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in
>> subclass overrides
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8258444: Explcitly inherit IOOBE in {Filter,InputStream}Reader
src/java.base/share/classes/java/io/PushbackReader.java line 98:
> 96: * {@inheritDoc}
> 97: */
> 98: public int read(char[] cbuf, int off, int len) throws IOException {
Shouldn't you add
* @throws IndexOutOfBoundException {@inheritDoc}
* @throws IOException {@inheritDoc}
here as well? IIRC the global {@inheritDoc} will not add them.
-
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides [v2]
On Tue, 23 Feb 2021 17:42:58 GMT, Brian Burkhalter wrote:
>> 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in
>> subclass overrides
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8258444: Explcitly inherit IOOBE in {Filter,InputStream}Reader
Nice cleanup.
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides [v2]
On Tue, 23 Feb 2021 18:00:33 GMT, Alan Bateman wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8258444: Explcitly inherit IOOBE in {Filter,InputStream}Reader
>
> Marked as reviewed by alanb (Reviewer).
CSR filed: https://bugs.openjdk.java.net/browse/JDK-8262262.
-
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null" [v2]
On Tue, 23 Feb 2021 19:35:55 GMT, Naoto Sato wrote: >> Please review the fix to this test case failure that occurs with the usage >> tracker enabled JRE. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review comments. LGTM! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2683
Integrated: 8253409: Double-rounding possibility in float fma
On Tue, 23 Feb 2021 03:58:46 GMT, Joe Darcy wrote: > In floating-point, usually doing an operation to double precision and then > rounding to float gives the right result in float precision. One exception to > this is fused multiply add (fma) where "a * b + c" is computed with a single > rounding. This requires the equivalent of extra intermediate precision inside > the operation. If a float fma is implemented using a double fma rounded to > float, for some well-chosen arguments where the final result is near a > half-way result in *float*, an incorrect answer will be computed due to > double rounding. In more detail, the double result will round up and then the > cast to float will round up again whereas a single rounding of the exact > answer to float would only round-up once. > > The new float fma implementation does the exact arithmetic using BigDecimal > where possible, with guard to handle the non-finite and signed zero IEEE 754 > details. This pull request has now been integrated. Changeset: e5304b3a Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/e5304b3a Stats: 32 lines in 2 files changed: 4 ins; 11 del; 17 mod 8253409: Double-rounding possibility in float fma Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk/pull/2684
Re: RFR: 8253409: Double-rounding possibility in float fma [v2]
On Tue, 23 Feb 2021 19:11:06 GMT, Brian Burkhalter wrote: > > > Looks fine. Presumably the updated test fails without the source change. Right; the added test case is the failing one from the bug report. It will fail if the old non-intrinsic implementation, that is the Java implementation is used. - PR: https://git.openjdk.java.net/jdk/pull/2684
Re: RFR: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null" [v2]
> Please review the fix to this test case failure that occurs with the usage > tracker enabled JRE. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflecting review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2683/files - new: https://git.openjdk.java.net/jdk/pull/2683/files/bfa3a8e5..4d82e8aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2683=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2683=00-01 Stats: 9 lines in 1 file changed: 1 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2683/head:pull/2683 PR: https://git.openjdk.java.net/jdk/pull/2683
Re: RFR: JDK-8262199: issue in jli args.c [v2]
On Tue, 23 Feb 2021 14:04:29 GMT, Christoph Langer wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Small changes > > src/java.base/share/native/libjli/args.c line 361: > >> 359: if (fptr != NULL) fclose(fptr); >> 360: exit(1); >> 361: } > > Can you insert a blank line here? This function is "optionally report, optionally fclose, and then exit". Have you tried reducing it to reportAndExit and fclose inline in expandArgFile to avoid it doing 3 things? - PR: https://git.openjdk.java.net/jdk/pull/2692
Re: RFR: 8253409: Double-rounding possibility in float fma [v2]
On Tue, 23 Feb 2021 07:00:07 GMT, Joe Darcy wrote: >> In floating-point, usually doing an operation to double precision and then >> rounding to float gives the right result in float precision. One exception >> to this is fused multiply add (fma) where "a * b + c" is computed with a >> single rounding. This requires the equivalent of extra intermediate >> precision inside the operation. If a float fma is implemented using a double >> fma rounded to float, for some well-chosen arguments where the final result >> is near a half-way result in *float*, an incorrect answer will be computed >> due to double rounding. In more detail, the double result will round up and >> then the cast to float will round up again whereas a single rounding of the >> exact answer to float would only round-up once. >> >> The new float fma implementation does the exact arithmetic using BigDecimal >> where possible, with guard to handle the non-finite and signed zero IEEE 754 >> details. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Add a jtreg run command to disable any fma instrinic so the Java code is > tested. Looks fine. Presumably the updated test fails without the source change. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2684
Re: RFR: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null"
On Tue, 23 Feb 2021 09:49:47 GMT, Daniel Fuchs wrote:
>> Please review the fix to this test case failure that occurs with the usage
>> tracker enabled JRE.
>
> test/jdk/java/util/Locale/LocaleProviders.java line 416:
>
>> 414: // Set the root logger on loading the logging class
>> 415: public static class LogConfig {
>> 416: final static LogRecord[] lra = new LogRecord[1];
>
> I would suggest to use some multi-thread safe container rather than a simple
> array to store the LogRecord. For instance - a CopyOnWriteArrayList - or
> something like that. Also you may want to harden the test by allowing for the
> possibility that some other logging might have occurred, and search the list
> for the record you expect rather than assuming it will be the first and only
> one.
> Otherwise looks good!
Thanks, Daniel. Will update the fix as you suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/2683
JDK-8262003: Class.arrayType should not throw IllegalArgumentException
I want to learn about writing a CSR, and need a sponsor teaching me the ropes. Bug: https://bugs.openjdk.java.net/browse/JDK-8262003 Currently, Class.arrayType() will throw an IllegalArgumentException if the maximum number of dimensions will be exceeded. Throwing an IllegalArgumentException from a method that doesn't take an argument is, at least, strange. Therefore I would like to update the specification to allow this method to throw an IllegalStateException, similar to what ClassDesc.arrayType() does. The current plan is: * Create a CSR detailing the spec changes: https://bugs.openjdk.java.net/browse/JDK-8262211 * Surround the code with a try-catch block. Checking for the error case is hard, and somewhat rare, so I think this is appropriate. * Copy the @throws javadoc line from ClassDesc.arrayType to Class.arrayType But there are a few questions I would love to get the answer on: * Both Class.arrayType() and ClassDesc.arrayType() are specified by TypeDescriptor.OfField. Should the specification of TypeDescriptor.OfField.arrayType() changed as well? * Is the draft of my CSR fine? Should I add some details, or omit things? Rephrase things? In advance, thanks for any support and feedback on this. - Johannes
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides [v2]
On Tue, 23 Feb 2021 17:42:58 GMT, Brian Burkhalter wrote:
>> 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in
>> subclass overrides
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8258444: Explcitly inherit IOOBE in {Filter,InputStream}Reader
Marked as reviewed by alanb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v7]
On Tue, 23 Feb 2021 12:15:08 GMT, Evan Whelan wrote: >> Hi, >> >> Please review this fix for JDK-8252883. This handles the case when an >> AccessDeniedException is being thrown on Windows, due to a delay in deleting >> the lock file it is trying to write to. >> >> This fix passes all testing. >> >> Kind regards, >> Evan > > Evan Whelan has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed copyright year in FileHandlerAccessTest.java Looks OK to me, although at my local Windows 10 box test took more than 100 iteration before it failed. - Marked as reviewed by vyomm...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides [v2]
> 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in
> subclass overrides
Brian Burkhalter has updated the pull request incrementally with one additional
commit since the last revision:
8258444: Explcitly inherit IOOBE in {Filter,InputStream}Reader
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2680/files
- new: https://git.openjdk.java.net/jdk/pull/2680/files/c96c80f0..687ad124
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2680=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2680=00-01
Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/2680.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2680/head:pull/2680
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v7]
On Tue, 23 Feb 2021 14:00:16 GMT, Evan Whelan wrote: >> Marked as reviewed by dfuchs (Reviewer). > > Thanks for the feedback Daniel! As I've already posted the integrate command, > I believe all this needs now is a sponsor :) Oops, never mind I've to re-issue the command - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides
On Tue, 23 Feb 2021 16:23:10 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/InputStreamReader.java line 167:
>>
>>> 165: * {@inheritDoc}
>>> 166: */
>>> 167: public int read(char[] cbuf, int off, int len) throws IOException {
>>
>> IOOBE is unchecked, are you sure it gets inherited into the sub-class here?
>
> The long standard behavior is that javadoc doesn't copy the throws of
> unchecked exceptions, instead we've had to declare the throws and use
> inheritDoc. I assume you'll need to do that here.
It does not and there is a javadoc issue already on file about this.
-
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v25]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 57 commits: - Merge branch 'master' into 8248862 - Adjust ThreadLocalRandom javadoc inheritence - L32X64StarStarRandom -> L32X64MixRandom - Various corrects - Revised javadoc per CSR reviews - Remove tabs from random/package-info.java - Correct copyright notice. - Merge branch 'master' into 8248862 - Update tests for RandomGeneratorFactory.all() - Merge branch 'master' into 8248862 - ... and 47 more: https://git.openjdk.java.net/jdk/compare/0257caad...b9094279 - Changes: https://git.openjdk.java.net/jdk/pull/1292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=24 Stats: 13693 lines in 26 files changed: 11542 ins; 2066 del; 85 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]
On Tue, 23 Feb 2021 16:18:29 GMT, Claes Redestad wrote: > > Is there any behavior change here that merits a CSR review? > > Maybe. The one observable change is that calling `Collections.bar(foo)` with > a `foo` that is already a `bar` will return the instance rather than > unnecessarily wrap it. This could change semantics in applications > inadvertently or deliberately relying on identity. Yes. The CSR was to consider primarily this case. Probably out of an abundance of caution here. @stuart-marks may have another case to consider. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides
On Tue, 23 Feb 2021 16:19:41 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/InputStreamReader.java line 167:
>>
>>> 165: * {@inheritDoc}
>>> 166: */
>>> 167: public int read(char[] cbuf, int off, int len) throws IOException {
>>
>> IOOBE is unchecked, are you sure it gets inherited into the sub-class here?
>
> It does not due to https://bugs.openjdk.java.net/browse/JDK-8157677.
The long standard behavior is that javadoc doesn't copy the throws of
unchecked exceptions, instead we've had to declare the throws and use
inheritDoc. I assume you'll need to do that here.
-
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides
On Tue, 23 Feb 2021 08:48:16 GMT, Alan Bateman wrote:
>> 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in
>> subclass overrides
>
> src/java.base/share/classes/java/io/InputStreamReader.java line 167:
>
>> 165: * {@inheritDoc}
>> 166: */
>> 167: public int read(char[] cbuf, int off, int len) throws IOException {
>
> IOOBE is unchecked, are you sure it gets inherited into the sub-class here?
It does not due to https://bugs.openjdk.java.net/browse/JDK-8157677.
-
PR: https://git.openjdk.java.net/jdk/pull/2680
Re: RFR: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized* [v4]
On Tue, 23 Feb 2021 06:12:54 GMT, Joe Darcy wrote: > Is there any behavior change here that merits a CSR review? Maybe. The one observable change is that calling `Collections.bar(foo)` with a `foo` that is already a `bar` will return the instance rather than unnecessarily wrap it. This could change semantics in applications inadvertently or deliberately relying on identity. - PR: https://git.openjdk.java.net/jdk/pull/2596
Re: RFR: JDK-8262199: TOCTOU in jli args.c [v2]
> Sonar reports a finding in args.c, where a file check is done . > Stat performs a check on file, and later fopen is called on the file : > https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8CL0BBG2CXpcnhtM=false=VULNERABILITY > > The coding could be slightly rewritten so that the potential TOCTOU is > removed (however I do not think that it is such a big issue). Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Small changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/2692/files - new: https://git.openjdk.java.net/jdk/pull/2692/files/78467273..8b106a14 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2692=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2692=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2692.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692 PR: https://git.openjdk.java.net/jdk/pull/2692
Re: RFR: JDK-8262199: TOCTOU in jli args.c [v2]
On Tue, 23 Feb 2021 14:30:17 GMT, Matthias Baesken wrote: >> Sonar reports a finding in args.c, where a file check is done . >> Stat performs a check on file, and later fopen is called on the file : >> https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8CL0BBG2CXpcnhtM=false=VULNERABILITY >> >> The coding could be slightly rewritten so that the potential TOCTOU is >> removed (however I do not think that it is such a big issue). > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Small changes Marked as reviewed by clanger (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2692
Re: RFR: JDK-8262199: TOCTOU in jli args.c [v2]
On Tue, 23 Feb 2021 14:23:59 GMT, Matthias Baesken wrote: > > This looks good in general. Do you know whether there's a jtreg test that > > stresses arg files? > > There are tests dealing with args files at test/jdk/tools/launcher/ , e.g. > there is test/jdk/tools/launcher/ArgsFileTest.java . > > Best regards, Matthias OK, I added label noreg-sqe to the bug then. - PR: https://git.openjdk.java.net/jdk/pull/2692
Re: RFR: JDK-8262199: TOCTOU in jli args.c
On Tue, 23 Feb 2021 14:05:15 GMT, Christoph Langer wrote: > This looks good in general. Do you know whether there's a jtreg test that > stresses arg files? There are tests dealing with args files at test/jdk/tools/launcher/ , e.g. there is test/jdk/tools/launcher/ArgsFileTest.java . Best regards, Matthias - PR: https://git.openjdk.java.net/jdk/pull/2692
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v24]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: L32X64StarStarRandom -> L32X64MixRandom - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/eeab6454..58a05f4c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=23 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=22-23 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v23]
> This PR is to introduce a new random number API for the JDK. The primary API > is found in RandomGenerator and RandomGeneratorFactory. Further description > can be found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Various corrects - Changes: - all: https://git.openjdk.java.net/jdk/pull/1292/files - new: https://git.openjdk.java.net/jdk/pull/1292/files/61f5d700..eeab6454 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1292=22 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1292=21-22 Stats: 34 lines in 4 files changed: 20 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/1292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: JDK-8262199: TOCTOU in jli args.c
On Tue, 23 Feb 2021 13:58:03 GMT, Matthias Baesken wrote: > Sonar reports a finding in args.c, where a file check is done . > Stat performs a check on file, and later fopen is called on the file : > https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8CL0BBG2CXpcnhtM=false=VULNERABILITY > > The coding could be slightly rewritten so that the potential TOCTOU is > removed (however I do not think that it is such a big issue). This looks good in general. Do you know whether there's a jtreg test that stresses arg files? src/java.base/share/native/libjli/args.c line 361: > 359: if (fptr != NULL) fclose(fptr); > 360: exit(1); > 361: } Can you insert a blank line here? - Changes requested by clanger (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2692
RFR: JDK-8262199: TOCTOU in jli args.c
Sonar reports a finding in args.c, where a file check is done . Stat performs a check on file, and later fopen is called on the file : https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8CL0BBG2CXpcnhtM=false=VULNERABILITY The coding could be slightly rewritten so that the potential TOCTOU is removed (however I do not think that it is such a big issue). - Commit messages: - JDK-8262199 Changes: https://git.openjdk.java.net/jdk/pull/2692/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2692=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262199 Stats: 32 lines in 1 file changed: 12 ins; 16 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2692.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2692/head:pull/2692 PR: https://git.openjdk.java.net/jdk/pull/2692
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v7]
On Tue, 23 Feb 2021 13:08:29 GMT, Daniel Fuchs wrote: >> Evan Whelan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed copyright year in FileHandlerAccessTest.java > > Marked as reviewed by dfuchs (Reviewer). Thanks for the feedback Daniel! As I've already posted the integrate command, I believe all this needs now is a sponsor :) - PR: https://git.openjdk.java.net/jdk/pull/2572
RE: Fast and cheap (Double|Float)::toString Java algorithm
>The last implementation is available in pre-Skara webrev form, as referenced >in [2] Hope to see it as a github review soon! Andrey Turbanov
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v7]
On Tue, 23 Feb 2021 12:15:08 GMT, Evan Whelan wrote: >> Hi, >> >> Please review this fix for JDK-8252883. This handles the case when an >> AccessDeniedException is being thrown on Windows, due to a delay in deleting >> the lock file it is trying to write to. >> >> This fix passes all testing. >> >> Kind regards, >> Evan > > Evan Whelan has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed copyright year in FileHandlerAccessTest.java Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v5]
On Tue, 23 Feb 2021 12:10:30 GMT, Evan Whelan wrote: >> Does the new version of the test still occasionally catches the issue and >> fails if the fix is not present? > > Hi, locally this test reproduces the issue on my Windows machine. > > I believe our internal testing Windows boxes are too powerful and handle the > threading too efficiently to reproduce this error. > > I can reproduce locally approx 10% of the time, but after over 100 runs I > cannot reproduce as part of our internal testing. > > As this is not verifiable in a completely deterministic way, I believe having > the test as a stable, passing test is a more appropriate approach instead of > selecting this as no-reg hard. > > It adds more test coverage to the code also. Reproducing locally with the test is fine. That's enough for me. - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v5]
On Tue, 23 Feb 2021 10:16:26 GMT, Daniel Fuchs wrote: >> Hi, >> >> I've removed the problematic "process" handling logic and have stripped the >> test back to only use threads. >> >> The problem was reproducible (intermittently on my Windows machine) using >> this smaller test and should make the test more stable. > > Does the new version of the test still occasionally catches the issue and > fails if the fix is not present? Hi, locally this test reproduces the issue on my Windows machine. I believe our internal testing Windows boxes are too powerful and handle the threading too efficiently to reproduce this error. I can reproduce locally approx 10% of the time, but after over 100 runs I cannot reproduce as part of our internal testing. As this is not verifiable in a completely deterministic way, I believe having the test as a stable, passing test is a more appropriate approach instead of selecting this as no-reg hard. It adds more test coverage to the code also. - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v7]
> Hi, > > Please review this fix for JDK-8252883. This handles the case when an > AccessDeniedException is being thrown on Windows, due to a delay in deleting > the lock file it is trying to write to. > > This fix passes all testing. > > Kind regards, > Evan Evan Whelan has updated the pull request incrementally with one additional commit since the last revision: Fixed copyright year in FileHandlerAccessTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/2572/files - new: https://git.openjdk.java.net/jdk/pull/2572/files/1bb8837e..c3b1d81d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2572=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2572=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2572.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2572/head:pull/2572 PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v6]
On Tue, 23 Feb 2021 10:14:07 GMT, Daniel Fuchs wrote: >> Evan Whelan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8252883: Stripped back FileHandlerAccessTest to only use threads > > test/jdk/java/util/logging/FileHandlerAccessTest.java line 2: > >> 1: /* >> 2: * Copyright (c) 2000, 2021, Oracle and/or its affiliates. All rights >> reserved. > > This seems like a new test - should it only have: > > * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. > ``` Thanks, will update - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8259267: Refactor LoaderLeak shell test as java test. [v11]
On Mon, 22 Feb 2021 20:35:19 GMT, Ivan Šipka wrote: >> Refactor `test/jdk/java/lang/annotation/loaderLeak/LoaderLeak.sh` as java >> test. > > Ivan Šipka has updated the pull request incrementally with one additional > commit since the last revision: > > 8166026: Refactor java/lang shell tests to java changes look good to me - though not strictly necessary, I wonder if you should null out the local variables that hold the class and classloader just after the reachability fence. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1577
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v6]
On Mon, 22 Feb 2021 09:50:06 GMT, Evan Whelan wrote: >> Hi, >> >> Please review this fix for JDK-8252883. This handles the case when an >> AccessDeniedException is being thrown on Windows, due to a delay in deleting >> the lock file it is trying to write to. >> >> This fix passes all testing. >> >> Kind regards, >> Evan > > Evan Whelan has updated the pull request incrementally with one additional > commit since the last revision: > > 8252883: Stripped back FileHandlerAccessTest to only use threads test/jdk/java/util/logging/FileHandlerAccessTest.java line 2: > 1: /* > 2: * Copyright (c) 2000, 2021, Oracle and/or its affiliates. All rights > reserved. This seems like a new test - should it only have: * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. ``` - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8252883: AccessDeniedException caused by delayed file deletion on Windows [v5]
On Mon, 22 Feb 2021 09:46:56 GMT, Evan Whelan wrote: >> I'm seeing some intermittent test failures with the new test (after merging >> in latest master changes). Can you retest and have a look: >> >> --System.out:(25/1343)-- >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Testing with arguments: type=process, count=20 >> Thread-2 |Error: Could not find or load main class FileHandlerAccessTest >> Thread-2 |Caused by: java.lang.ClassNotFoundException: >> FileHandlerAccessTest >> Thread-6 |Error: Could not find or load main class FileHandlerAccessTest >> Thread-6 |Caused by: java.lang.ClassNotFoundException: >> FileHandlerAccessTest >> Thread-1 |Error: Could not find or load main class FileHandlerAccessTest >> Thread-1 |Caused by: java.lang.ClassNotFoundException: >> FileHandlerAccessTest >> Testing with arguments: type=process, count=20 >> --System.err:(14/1024)-- >> java.lang.RuntimeException: java.lang.RuntimeException: An error occured in >> the child process. >> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96) >> at java.base/java.lang.Thread.run(Thread.java:831) >> Caused by: java.lang.RuntimeException: An error occured in the child process. >> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93) >> ... 1 more >> STATUS:Failed.`main' threw exception: java.lang.RuntimeException: >> java.lang.RuntimeException: An error occured in the child process. >> java.lang.RuntimeException: java.lang.RuntimeException: An error occured in >> the child process. >> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:96) >> at java.base/java.lang.Thread.run(Thread.java:831) >> Caused by: java.lang.RuntimeException: An error occured in the child process. >> at FileHandlerAccessTest.accessProcess(FileHandlerAccessTest.java:93) >> ... 1 more >> STATUS:Failed.`main' threw exception: java.lang.RuntimeException: >> java.lang.RuntimeException: An error occured in the child process. >> --rerun:(39/6191)*-- > > Hi, > > I've removed the problematic "process" handling logic and have stripped the > test back to only use threads. > > The problem was reproducible (intermittently on my Windows machine) using > this smaller test and should make the test more stable. Does the new version of the test still occasionally catches the issue and fails if the fix is not present? - PR: https://git.openjdk.java.net/jdk/pull/2572
Re: RFR: 8261919: java/util/Locale/LocaleProvidersRun.java failed with "RuntimeException: Expected log was not emitted. LogRecord: null"
On Tue, 23 Feb 2021 02:09:01 GMT, Naoto Sato wrote:
> Please review the fix to this test case failure that occurs with the usage
> tracker enabled JRE.
test/jdk/java/util/Locale/LocaleProviders.java line 416:
> 414: // Set the root logger on loading the logging class
> 415: public static class LogConfig {
> 416: final static LogRecord[] lra = new LogRecord[1];
I would suggest to use some multi-thread safe container rather than a simple
array to store the LogRecord. For instance - a CopyOnWriteArrayList - or
something like that. Also you may want to harden the test by allowing for the
possibility that some other logging might have occurred, and search the list
for the record you expect rather than assuming it will be the first and only
one.
Otherwise looks good!
-
PR: https://git.openjdk.java.net/jdk/pull/2683
Re: RFR: 8258444: Clean up specifications of java.io.Reader.read(char[], int, int) in subclass overrides
On Mon, 22 Feb 2021 23:27:19 GMT, Brian Burkhalter wrote:
> 8258444: Clean up specifications of java.io.Reader.read(char[],int,int) in
> subclass overrides
Marked as reviewed by alanb (Reviewer).
src/java.base/share/classes/java/io/InputStreamReader.java line 167:
> 165: * {@inheritDoc}
> 166: */
> 167: public int read(char[] cbuf, int off, int len) throws IOException {
IOOBE is unchecked, are you sure it gets inherited into the sub-class here?
-
PR: https://git.openjdk.java.net/jdk/pull/2680
