Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v2]
On Wed, 26 Jun 2024 15:40:36 GMT, SendaoYan wrote:
>> Hi all,
>> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails
>> on rpm build mock environment. The `df -h` command return fail `df: cannot
>> read table of mounted file systems: No such file or directory` on the rpm
>> build mock environment also. I think it's a environmental issue, and the
>> environmental issue should not cause the test fails, it should skip the test.
>>
>> Only change the testcase, the change has been verified locally, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add a word throw
test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java line 216:
> 214: } catch (IOException e) {
> 215: if (e.getMessage().contains("Mount point not found")) {
> 216: // We would like to skip the test with a cause with
Hello @sendaoYan, it feels very specific and odd to be checking only for this
exception message. This test method's goal appears to be to create a read-only
directory into which it wants to write out the proxy classes and verify that it
won't be able to do that. For that it first verifies that the underlying
`FileStore` supports posix file attributes. If it's not able to ascertain that
the underlying `FileStore` has posix support, then it skips the test.
So I think we should just catch the `IOException` here when getting the
FileStore and skip the test instead of checking for specific exception
messages. While we are at it, we should throw a `org.testng.SkipException`
(this is a testng test) from this method wherever we are currently skipping the
test execution by writing out a System.out warning message and returning.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19905#discussion_r1690807453
Re: RFR: 8334394: Race condition in Class::protectionDomain [v3]
On Mon, 22 Jul 2024 12:42:48 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with two additional > commits since the last revision: > > - var to real type > - rename Still looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2191565356
Re: RFR: 8336844: ZipConstants64 defines duplicate constants EXTID_ZIP64 and ZIP64_EXTID [v2]
On Sat, 20 Jul 2024 02:18:11 GMT, Liam Miller-Cushon wrote: >> This change deduplicates constants in ZipConstants64 for the Zip64 Extended >> Information Extra Field Header ID (see APPNOTE.TXT 4.5.3). >> >> I think there are arguments for consolidating on either `EXTID_ZIP64` or >> `ZIP64_EXTID`. The PR currently consolidates on `EXTID_ZIP64`, `ZIP64_EXTID` >> is also an option if there's a preference for that. >> >> I noticed this while working on a zip64 bug in >> [JDK-8328995](https://bugs.openjdk.org/browse/JDK-8328995), I was reviewing >> places that handled zip64 extra fields and initially missed some because I >> was only looking at one of the constants. > > Liam Miller-Cushon 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 three additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into zip64_extid > - Update copyright year > - 8336844: ZipConstants64 defines duplicate constants EXTID_ZIP64 and > EXTID_ZIP64. The changes look good to me. Although a trivial change, it would be good to wait for Lance to review this before integrating. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20264#pullrequestreview-2189766637
Re: RFR: 8336844: ZipConstants64 defines duplicate constants EXTID_ZIP64 and ZIP64_EXTID
On Fri, 19 Jul 2024 19:35:14 GMT, Liam Miller-Cushon wrote: > This change deduplicates constants in ZipConstants64 for the Zip64 Extended > Information Extra Field Header ID (see APPNOTE.TXT 4.5.3). > > I think there are arguments for consolidating on either `EXTID_ZIP64` or > `ZIP64_EXTID`. The PR currently consolidates on `EXTID_ZIP64`, `ZIP64_EXTID` > is also an option if there's a preference for that. > > I noticed this while working on a zip64 bug in > [JDK-8328995](https://bugs.openjdk.org/browse/JDK-8328995), I was reviewing > places that handled zip64 extra fields and initially missed some because I > was only looking at one of the constants. Hello Liam, the changes look fine to me. Choosing `EXTID_ZIP64` constant in preference of the other seems OK too, since that constant is more closer in code location with the other extra field header ids. Some of these files require a copyright year update. Please update those before integrating. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20264#pullrequestreview-2189609923
Re: RFR: 8328995: Launcher can't open jar files where the offset of the manifest is >4GB [v8]
On Thu, 18 Jul 2024 21:36:54 GMT, Liam Miller-Cushon wrote:
>> This change fixes a zip64 bug in the launcher that is prevent it from
>> reading the manifest of jars where the 'relative offset of local header'
>> field in the central directory entry is >4GB. As described in APPNOTE.TXT
>> 4.5.3, the offset is too large to be stored in the central directory it is
>> stored in a 'Zip64 Extended Information Extra Field'.
>
> Liam Miller-Cushon 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:
>
> - Add a missing `break`
> - Merge branch 'master' into JDK-8328995
> - Merge remote-tracking branch 'origin/master' into JDK-8328995
> - Move test to test/jdk/tools/launcher
> - Add some more comments
> - Maximum Zip64 extra field length is 32
> - Make cendsk an unsigned short
> - Fix disk number size
> - Improvements
>
>* don't rely on variable length arrays
>* only run the test of 64 bit machines, since it requires >4GB of heap
> - 8328995: launcher can't open jar files where the offset of the manifest is
> >4GB
src/java.base/share/native/libjli/parse_manifest.c line 507:
> 505: || censiz == ZIP64_MAGICVAL
> 506: || cenoff == ZIP64_MAGICVAL)
> 507: && cenext > 0) {
I went through these changes and here's my first round of review.
Before the changes proposed in this PR, this part of the code which is
responsible for finding and returning a constructed zip entry instance would
blindly use the local header offset value from the central directory of the
entry being looked up. It would then "seek" to that position and read the
metadata of that entry (details like uncompressed length, compressed length,
the compression method) and return back that entry instance. Clearly this isn't
right when zip64 entries are involved since various details of such an entry
can reside in the zip64 extra field block of that entry instead of being in the
central directory.
This change proposes that this part of the code first identify that a zip64
extra block exists for a particular entry and then read that zip64 block,
validate some parts of the zip64 block and if that validation succeeds, then
use the entry metadata from the zip64 block for constructing and returning the
entry instance.
The idea to identify and support zip64 entries in this part of the code I think
is agreed as the right one.
Coming to the implementation, I think we need to come to an agreement on what
we want to do here. Specifically:
- What logic do we use to decide when to look for zip64 extra block for an
entry? The changes in this PR (at this line here) proposes that we look for
zip64 extra block for an entry if any of the compressed size, uncompressed size
or the local header offset value is `0xL` and the extra field size
noted in this central directory entry is greater than 0. This however doesn't
match with what we do in the implementation of `java.util.zip.ZipFile` which
too does similar checks for zip64 entry presence when parsing the central
directory. Very specifically, in the ZipFile where this logic is implemented is
here
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/ZipFile.java#L1254.
That code in the ZipFile has gone through the necessary vetting for dealing
with various possibilities with the zip files. I think we should implement that
same logic here while checking for zip64 entries.
- The next one to decide is what kind of validations do we want to do in this
code for zip64 extra field block. I think here too we should match whatever w
currently do in the `java.util.zip.ZipFile` implementation, specifically what's
being done in the `checkExtraFields` and `checkZip64ExtraFieldValues` methods
of that class.
- Next, what do we do when these validations fail. Right now in this proposed
implementation, we appear to consider some validation failures as the entry
being absent. My opinion is that we should be more stricter and fail the jar
like we do in the implementation of `ZipFile` for such validation failures.
One additional thing that this proposed change does is that it validates that
the local header offset determined for an entry (either through the central
directory or through the zip64 extra field block) does indeed point to a local
header for an entry with the same file name. I don't think we do that in the
ZipFile implementation. But I think doing that check in this code is fine.
There are few other implementation details that I haven't commented about
because they may not matter depending on what conclusions we arrive at for the
above few points.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1684450259
Re: RFR: 8328995: Launcher can't open jar files where the offset of the manifest is >4GB [v8]
On Thu, 18 Jul 2024 21:36:54 GMT, Liam Miller-Cushon wrote: >> This change fixes a zip64 bug in the launcher that is prevent it from >> reading the manifest of jars where the 'relative offset of local header' >> field in the central directory entry is >4GB. As described in APPNOTE.TXT >> 4.5.3, the offset is too large to be stored in the central directory it is >> stored in a 'Zip64 Extended Information Extra Field'. > > Liam Miller-Cushon 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: > > - Add a missing `break` > - Merge branch 'master' into JDK-8328995 > - Merge remote-tracking branch 'origin/master' into JDK-8328995 > - Move test to test/jdk/tools/launcher > - Add some more comments > - Maximum Zip64 extra field length is 32 > - Make cendsk an unsigned short > - Fix disk number size > - Improvements > >* don't rely on variable length arrays >* only run the test of 64 bit machines, since it requires >4GB of heap > - 8328995: launcher can't open jar files where the offset of the manifest is > >4GB src/java.base/share/native/libjli/parse_manifest.c line 186: > 184: } > 185: > 186: /** Nit: For the sake of consistency with the rest of the comments in this file, this comment should be a `/*` instead of `/**` - PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1684376439
Re: RFR: 8336479: Provide Process.waitFor(Duration) [v5]
On Thu, 18 Jul 2024 20:51:48 GMT, Naoto Sato wrote: >> Proposing a new overload method for `Process#waitFor()` which takes a >> `Duration` for the timeout value. This will reduce the possibility for >> making mistakes with the `TimeUnit` in the other overload method. A >> corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > removed a blank line Thank you Naoto, the updated changes look good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20220#pullrequestreview-2187267936
Re: RFR: 8334771: [TESTBUG] Run TestDockerMemoryMetrics.java with -Xcomp fails exitValue = 137
On Thu, 18 Jul 2024 11:42:17 GMT, SendaoYan wrote: >> Hi all, >> After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the >> footprint memory usage increased significantly when run the testcase with >> -Xcomp jvm options, then cause the testcase was killed by docker by OOM. >> Maybe the footprint memory usage increased was inevitable, so I think we >> should increase the smallest memory limite for this testcase. >> Only change the testcase, the change has been verified, no risk. > >> Unfortunately I'm not familiar with these tests. > >> After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the >> codecache usage increased significantly, non-profiled 3068Kb->3583Kb, >> profiled 6408Kb->7846Kb. > > Can you confirm that the codecache usage increased is expected or not after > [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960) with -Xcomp jvm > option. @sendaoYan, Given Adam's inputs and the reviews you have had for this change, I think you should be able to go ahead and integrate this. - PR Comment: https://git.openjdk.org/jdk/pull/19864#issuecomment-2238072647
Re: RFR: 4452735: Add GZIPOutputStream constructor to specify Deflater
On Wed, 17 Jul 2024 21:07:23 GMT, Archie Cobbs wrote: > The class `GZIPOutputStream` extends `DeflaterOutputStream`, which is logical > because the GZIP encoding is based on ZLIB "deflate" encoding. > > However, while `DeflaterOutputStream` provides constructors that take a > custom `Deflater` argument supplied by the caller, `GZIPOutputStream` has no > such constructors. > > As a result, it's not possible to do entirely reasonable customization, such > as configuring a `GZIPOutputStream` for a non-default compression level. > > This change adds a new `GZIPOutputStream` constructor that accepts a custom > `Deflater`, and also adds a basic unit test for it and all of the other > `GZIPOutputStream` constructors, based on the existing test > `BasicGZIPInputStreamTest.java` which does the same thing for > `GZIPInputStream`. Hello Chen, yes this PR and another one from Archie in this area is on my list to review. I'll be getting to this one shortly. - PR Comment: https://git.openjdk.org/jdk/pull/20226#issuecomment-2238060585
Re: RFR: 8328995: Launcher can't open jar files where the offset of the manifest is >4GB [v7]
On Tue, 4 Jun 2024 17:43:24 GMT, Liam Miller-Cushon wrote: >> This change fixes a zip64 bug in the launcher that is prevent it from >> reading the manifest of jars where the 'relative offset of local header' >> field in the central directory entry is >4GB. As described in APPNOTE.TXT >> 4.5.3, the offset is too large to be stored in the central directory it is >> stored in a 'Zip64 Extended Information Extra Field'. > > Liam Miller-Cushon 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 eight additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into JDK-8328995 > - Move test to test/jdk/tools/launcher > - Add some more comments > - Maximum Zip64 extra field length is 32 > - Make cendsk an unsigned short > - Fix disk number size > - Improvements > >* don't rely on variable length arrays >* only run the test of 64 bit machines, since it requires >4GB of heap > - 8328995: launcher can't open jar files where the offset of the manifest is > >4GB src/java.base/share/native/libjli/parse_manifest.c line 520: > 518: free(buffer); > 519: return (-1); > 520: } I think we are missing a `break;` here, after this inner `if` block reads the zip64 extra fields and returns `JNI_TRUE`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18479#discussion_r1682999880
Re: RFR: 8334771: [TESTBUG] Run TestDockerMemoryMetrics.java with -Xcomp fails exitValue = 137
On Mon, 24 Jun 2024 16:16:29 GMT, SendaoYan wrote: > After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the > footprint memory usage increased significantly when run the testcase with > -Xcomp jvm options, then cause the testcase was killed by docker by OOM. Maybe the footprint memory usage increased was inevitable, so I think we should increase the smallest memory limite for this testcase. Hello @sendaoYan, after changes in JDK-8294960, there were a couple of issues reported. From what I see in the linked issues, Adam reviewed those and integrated relevant fixes. In JDK-8334771 you note: > After [JDK-8294960](https://bugs.openjdk.org/browse/JDK-8294960), the > codecache usage increased significantly, non-profiled 3068Kb->3583Kb, > profiled 6408Kb->7846Kb. So I think we should have this increase in memory reviewed by @asotona or someone familiar in that area, before deciding whether these tests should be changed. - PR Comment: https://git.openjdk.org/jdk/pull/19864#issuecomment-2235607869
Re: RFR: 8336479: Provide Process.waitFor(Duration) [v2]
On Wed, 17 Jul 2024 21:41:16 GMT, Naoto Sato wrote:
>> Proposing a new overload method for `Process#waitFor()` which takes a
>> `Duration` for the timeout value. This will reduce the possibility for
>> making mistakes with the `TimeUnit` in the other overload method. A
>> corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> ProcessTools overriding one-arg waitFor()
test/jdk/java/lang/Process/WaitForDuration.java line 57:
> 55: throws IOException, InterruptedException {
> 56: assertEquals(expected,
> 57: new ProcessBuilder("sleep", "3").start().waitFor(d));
I think in its current form, this has a chance of failure (for inputs like 0 or
negative duration), if the sleep (for 3 seconds) completes (and thus the
process exits) before the `Process.waitFor` implementation has had a chance to
execute `hasExited()`.
Also, this test is marked to run on all platforms. I think we might need
special handling for `sleep` executable on Windows. In fact, looking at the
`initSleepPath` in the `test/jdk/java/lang/ProcessBuilder/Basic.java` test, I
suspect we might need something similar in this test even for *nix.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20220#discussion_r1682143756
Re: RFR: 8336479: Provide Process.waitFor(Duration) [v2]
On Wed, 17 Jul 2024 21:41:16 GMT, Naoto Sato wrote: >> Proposing a new overload method for `Process#waitFor()` which takes a >> `Duration` for the timeout value. This will reduce the possibility for >> making mistakes with the `TimeUnit` in the other overload method. A >> corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > ProcessTools overriding one-arg waitFor() src/java.base/share/classes/java/lang/Process.java line 499: > 497: Objects.requireNonNull(duration, "duration"); // throw NPE > before other conditions > 498: > 499: if (hasExited()) For clarity, the javadoc on the private internal `hasExited()` method might need an update - it currently states that the method will be invoked from `waitFor(long, TimeUnit)`. - PR Review Comment: https://git.openjdk.org/jdk/pull/20220#discussion_r1682133588
Re: RFR: 8336479: Provide Process.waitFor(Duration) [v2]
On Wed, 17 Jul 2024 21:41:16 GMT, Naoto Sato wrote: >> Proposing a new overload method for `Process#waitFor()` which takes a >> `Duration` for the timeout value. This will reduce the possibility for >> making mistakes with the `TimeUnit` in the other overload method. A >> corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > ProcessTools overriding one-arg waitFor() src/java.base/share/classes/java/lang/Process.java line 501: > 499: if (hasExited()) > 500: return true; > 501: if (duration.isZero() || duration.isNegative()) Hello Naoto, I see that there's a `Duration.isPositive()` API. Should we use that here instead? - PR Review Comment: https://git.openjdk.org/jdk/pull/20220#discussion_r1682126873
Re: RFR: 8334057: JLinkReproducibleTest.java support receive test.tool.vm.opts [v2]
On Mon, 15 Jul 2024 15:29:03 GMT, SendaoYan wrote: > Does this PR need 2rd reviewer. core-libs area doesn't mandate 2 reviews. The current PR is a test infrastructure change and doesn't impact the functionality of the test. The change has been tested in our CI and appears to work fine without introducing any regressions. Plus the PR has been open for more than 24 hours. So I think it is OK to issue a "integrate" whenever you are ready. - PR Comment: https://git.openjdk.org/jdk/pull/19669#issuecomment-2228859627
Re: RFR: 8173970: jar tool should have a way to extract to a directory [v8]
> Can I please get a review for this patch which proposes to implement the > enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970? > > The commit in this PR introduces the `-o` and `--output-dir` option to the > `jar` command. The option takes a path to a destination directory as a value > and extracts the contents of the jar into that directory. This is an optional > option and the changes in the commit continue to maintain backward > compatibility where the jar is extracted into current directory, if no `-o` > or `--output-dir` option has been specified. > > As far as I know, there hasn't been any discussion on what the name of this > new option should be. I was initially thinking of using `-d` but that is > currently used by the `jar` command for a different purpose. So I decided to > use `-o` and `--output-dir`. This is of course open for change depending on > any suggestions in this PR. > > The commit in this PR also updates the `jar.properties` file which contains > the English version of the jar command's `--help` output. However, no changes > have been done to the internationalization files corresponding to this one > (for example: `jar_de.properties`), because I don't know what process needs > to be followed to have those files updated (if at all they need to be > updated). > > The commit also includes a jtreg testcase which verifies the usage of this > new option. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - merge latest from master branch - merge latest from master branch - merge latest from master branch - cleanup testExtractNoDestDirWithPFlag() test - merge latest from master branch - merge latest from master branch - convert the new test to junit - merge latest from master branch - Lance's review - include tests for --extract long form option - cleanup after each test - ... and 8 more: https://git.openjdk.org/jdk/compare/a253e0ff...ece49f9f - Changes: https://git.openjdk.org/jdk/pull/2752/files Webrev: https://webrevs.openjdk.org/?repo=jdk=2752=07 Stats: 569 lines in 4 files changed: 558 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/2752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/2752/head:pull/2752 PR: https://git.openjdk.org/jdk/pull/2752
Re: RFR: 8322256: Define and document GZIPInputStream concatenated stream semantics [v5]
On Mon, 15 Jul 2024 10:06:20 GMT, Jaikiran Pai wrote:
>> Archie Cobbs has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 12 commits:
>>
>> - Bump @since from 23 to 24.
>> - Merge branch 'master' into JDK-8322256
>> - Relabel "trailing garbage" as "extra bytes" to sound less accusatory.
>> - Simplify code by eliminating an impossible case.
>> - Field name change and Javadoc wording tweaks.
>> - Merge branch 'master' into JDK-8322256
>> - Javadoc wording tweaks.
>> - Merge branch 'master' into JDK-8322256
>> - Clarify exceptions: sometimes ZipException, sometimes EOFException.
>> - Merge branch 'master' into JDK-8322256
>> - ... and 2 more: https://git.openjdk.org/jdk/compare/75dc2f85...f845a75b
>
> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 153:
>
>> 151: */
>> 152: public GZIPInputStream(InputStream in, int size,
>> 153: boolean allowConcatenation, boolean ignoreExtraBytes)
>> throws IOException {
>
> I haven't reviewed the javadoc changes. I will do that separately once we
> settle down on the API and implementation changes.
> As for this new constructor, I think the only new parameter we should
> introduce is whether or not the underlying input stream `in` is
> expected/allowed to have concatenated GZIP stream(s). So I think we should
> remove the "ignoreExtraBytes" new parameters from this constructor and.
> Additionally, I think the `allowConcatenation` should instead be named
> `allowConcatenatedGZIPStream`:
>
>
> public GZIPInputStream(InputStream in, int size, boolean
> allowConcatenatedGZIPStream) throws IOException {
Just to provide a more concrete input, here's a very minimal tested version of
what I had in mind:
--- a/src/java.base/share/classes/java/util/zip/GZIPInputStream.java
+++ b/src/java.base/share/classes/java/util/zip/GZIPInputStream.java
@@ -55,6 +55,8 @@ public class GZIPInputStream extends InflaterInputStream {
private boolean closed = false;
+private final boolean allowConcatenatedGZIPStream;
+
/**
* Check to make sure that this stream has not been closed
*/
@@ -76,14 +78,7 @@ private void ensureOpen() throws IOException {
* @throwsIllegalArgumentException if {@code size <= 0}
*/
public GZIPInputStream(InputStream in, int size) throws IOException {
-super(in, createInflater(in, size), size);
-usesDefaultInflater = true;
-try {
-readHeader(in);
-} catch (IOException ioe) {
-this.inf.end();
-throw ioe;
-}
+this(in, size, true);
}
/*
@@ -111,7 +106,28 @@ private static Inflater createInflater(InputStream in, int
size) {
* @throwsIOException if an I/O error has occurred
*/
public GZIPInputStream(InputStream in) throws IOException {
-this(in, 512);
+this(in, 512, true);
+}
+
+/**
+ * WIP
+ * @param in the input stream
+ * @param size the input buffer size
+ * @param allowConcatenatedGZIPStream true if the input stream is allowed
to contain
+ *concatenated GZIP streams. false
otherwise
+ * @throws IOException if an I/O error has occurred
+ */
+public GZIPInputStream(InputStream in, int size, boolean
allowConcatenatedGZIPStream)
+throws IOException {
+super(in, createInflater(in, size), size);
+this.allowConcatenatedGZIPStream = allowConcatenatedGZIPStream;
+usesDefaultInflater = true;
+try {
+readHeader(in);
+} catch (IOException ioe) {
+this.inf.end();
+throw ioe;
+}
}
/**
@@ -150,10 +166,45 @@ public int read(byte[] buf, int off, int len) throws
IOException {
}
int n = super.read(buf, off, len);
if (n == -1) {
-if (readTrailer())
+// reading of deflated data is now complete, we now read the GZIP
stream trailer
+readTrailer();
+if (!allowConcatenatedGZIPStream) {
eos = true;
-else
+} else {
+// This GZIPInputStream instance was created to allow potential
+// concatenated GZIP stream, so we now try and read the next
+// GZIP stream header, if any.
+
+// use any un-inflated remaining bytes from the stream
+InputStream headerIS = in;
+int remainingUnInflated = inf.getRemaining();
+if (remainingUnInflated > 0) {
+headerIS = new SequenceInputStream(
+new ByteArrayInputStream(this.buf, this.len -
remainingUnInfla
Re: RFR: 8322256: Define and document GZIPInputStream concatenated stream semantics [v5]
On Thu, 6 Jun 2024 17:03:57 GMT, Archie Cobbs wrote:
>> `GZIPInputStream` supports reading data from multiple concatenated GZIP data
>> streams since [JDK-4691425](https://bugs.openjdk.org/browse/JDK-4691425). In
>> order to do this, after a GZIP trailer frame is read, it attempts to read a
>> GZIP header frame and, if successful, proceeds onward to decompress the new
>> stream. If the attempt to decode a GZIP header frame fails, or happens to
>> trigger an `IOException`, it just ignores the trailing garbage and/or the
>> `IOException` and returns EOF.
>>
>> There are several issues with this:
>>
>> 1. The behaviors of (a) supporting concatenated streams and (b) ignoring
>> trailing garbage are not documented, much less precisely specified.
>> 2. Ignoring trailing garbage is dubious because it could easily hide errors
>> or other data corruption that an application would rather be notified about.
>> Moreover, the API claims that a `ZipException` will be thrown when corrupt
>> data is read, but obviously that doesn't happen in the trailing garbage
>> scenario (so N concatenated streams where the last one has a corrupted
>> header frame is indistinguishable from N-1 valid streams).
>> 3. There's no way to create a `GZIPInputStream` that does _not_ support
>> stream concatenation.
>>
>> On the other hand, `GZIPInputStream` is an old class with lots of existing
>> usage, so it's important to preserve the existing behavior, warts and all
>> (note: my the definition of "existing behavior" here includes the bug fix in
>> [JDK-7036144](https://bugs.openjdk.org/browse/JDK-7036144)).
>>
>> So this patch adds a new constructor that takes two new parameters
>> `allowConcatenation` and `allowTrailingGarbage`. The legacy behavior is
>> enabled by setting both to true; otherwise, they do what they sound like. In
>> particular, when `allowTrailingGarbage` is false, then the underlying input
>> must contain exactly one (if `allowConcatenation` is false) or exactly N (if
>> `allowConcatenation` is true) concatenated GZIP data streams, otherwise an
>> exception is guaranteed.
>
> Archie Cobbs has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 12 commits:
>
> - Bump @since from 23 to 24.
> - Merge branch 'master' into JDK-8322256
> - Relabel "trailing garbage" as "extra bytes" to sound less accusatory.
> - Simplify code by eliminating an impossible case.
> - Field name change and Javadoc wording tweaks.
> - Merge branch 'master' into JDK-8322256
> - Javadoc wording tweaks.
> - Merge branch 'master' into JDK-8322256
> - Clarify exceptions: sometimes ZipException, sometimes EOFException.
> - Merge branch 'master' into JDK-8322256
> - ... and 2 more: https://git.openjdk.org/jdk/compare/75dc2f85...f845a75b
src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 153:
> 151: */
> 152: public GZIPInputStream(InputStream in, int size,
> 153: boolean allowConcatenation, boolean ignoreExtraBytes) throws
> IOException {
I haven't reviewed the javadoc changes. I will do that separately once we
settle down on the API and implementation changes.
As for this new constructor, I think the only new parameter we should introduce
is whether or not the underlying input stream `in` is expected/allowed to have
concatenated GZIP stream(s). So I think we should remove the "ignoreExtraBytes"
new parameters from this constructor and. Additionally, I think the
`allowConcatenation` should instead be named `allowConcatenatedGZIPStream`:
public GZIPInputStream(InputStream in, int size, boolean
allowConcatenatedGZIPStream) throws IOException {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18385#discussion_r1677592647
Re: RFR: 8322256: Define and document GZIPInputStream concatenated stream semantics [v5]
On Thu, 6 Jun 2024 17:03:57 GMT, Archie Cobbs wrote: >> `GZIPInputStream` supports reading data from multiple concatenated GZIP data >> streams since [JDK-4691425](https://bugs.openjdk.org/browse/JDK-4691425). In >> order to do this, after a GZIP trailer frame is read, it attempts to read a >> GZIP header frame and, if successful, proceeds onward to decompress the new >> stream. If the attempt to decode a GZIP header frame fails, or happens to >> trigger an `IOException`, it just ignores the trailing garbage and/or the >> `IOException` and returns EOF. >> >> There are several issues with this: >> >> 1. The behaviors of (a) supporting concatenated streams and (b) ignoring >> trailing garbage are not documented, much less precisely specified. >> 2. Ignoring trailing garbage is dubious because it could easily hide errors >> or other data corruption that an application would rather be notified about. >> Moreover, the API claims that a `ZipException` will be thrown when corrupt >> data is read, but obviously that doesn't happen in the trailing garbage >> scenario (so N concatenated streams where the last one has a corrupted >> header frame is indistinguishable from N-1 valid streams). >> 3. There's no way to create a `GZIPInputStream` that does _not_ support >> stream concatenation. >> >> On the other hand, `GZIPInputStream` is an old class with lots of existing >> usage, so it's important to preserve the existing behavior, warts and all >> (note: my the definition of "existing behavior" here includes the bug fix in >> [JDK-7036144](https://bugs.openjdk.org/browse/JDK-7036144)). >> >> So this patch adds a new constructor that takes two new parameters >> `allowConcatenation` and `allowTrailingGarbage`. The legacy behavior is >> enabled by setting both to true; otherwise, they do what they sound like. In >> particular, when `allowTrailingGarbage` is false, then the underlying input >> must contain exactly one (if `allowConcatenation` is false) or exactly N (if >> `allowConcatenation` is true) concatenated GZIP data streams, otherwise an >> exception is guaranteed. > > Archie Cobbs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 12 commits: > > - Bump @since from 23 to 24. > - Merge branch 'master' into JDK-8322256 > - Relabel "trailing garbage" as "extra bytes" to sound less accusatory. > - Simplify code by eliminating an impossible case. > - Field name change and Javadoc wording tweaks. > - Merge branch 'master' into JDK-8322256 > - Javadoc wording tweaks. > - Merge branch 'master' into JDK-8322256 > - Clarify exceptions: sometimes ZipException, sometimes EOFException. > - Merge branch 'master' into JDK-8322256 > - ... and 2 more: https://git.openjdk.org/jdk/compare/75dc2f85...f845a75b Hello Archie, sorry it has taken long to review this PR. The proposal here is to have `java.util.zip.GZIPInputStream` specify (and improve) how it deals with concatenated GZIP stream(s) and also allow for applications instantiating `GZIPInputStream` to decide whether or not the underlying `InputStream` passed to the `GZIPInputStream` instance is expected to contain a concatenated GZIP stream(s). I'll include here the text that I had sent in a private communication to Archie: I think this comes down to introducing a new optional boolean parameter to the constructor of GZIPInputStream. The boolean when "true" will attempt to read a potential additional GZIP stream after the previous GZIP stream's trailer has been read. In that case we need to handle 2 cases - one where we successfully find the header of the next (concatenated) GZIP stream and the other where we don't find a valid GZIP stream header. In the first case where we do find a header successfully, then we continue reading the stream as usual and return the uncompressed data from the "read()" call. In the case where we fail to find a valid header and yet there were bytes past the previous GZIP stream, then I think the GZIPInputStream.read() should throw an IOException, since that stream no longer represents a GZIP stream (remember, we are talking about this only when the GZIPInputStream was constructed with the new boolean parameter = true). Coming to the case where the GZIPInputStream was constructed using boolean = false - In this case when we reach and read the trailer of a GZIP stream, if there is no more bytes, then we consider this a completed GZIP stream and return the uncompressed data. If there is any more bytes past the GZIP stream's trailer, then I think we should throw a IOException (since we aren't expecting any concatenated GZIP stream). As for the default value of this optional boolean parameter, I think it should default to "true" implying it will read any concatenated GZIP streams. That would match the current implementation of GZIPInputStream.read() which has the ability to read (valid) GZIP concatenated input stream. I
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v3]
On Sun, 16 Jun 2024 06:36:00 GMT, Jaikiran Pai wrote: >> Can I please get a review of this enhancement which proposes to enhance >> `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`? >> >> The actual work for this was done a few years back when we discussed the >> proposed approaches and then I raised a RFR. At that time I couldn't take >> this to completion. The current changes in this PR involve the >> implementation that was discussed at that time and also have implemented the >> review suggestions from that time. Here are those previous discussions and >> reviews: >> >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html >> https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html >> >> To summarize those discussions, we had concluded that: >> - `Deflater` and `Inflater` will implement the `AutoCloseable` interface >> - In the `close()` implementation we will invoke the `end()` method >> (`end()` can be potentially overridden by subclasses). >> - `close()` will be specified and implemented to be idempotent. Calling >> `close()` a second time or more will be a no-op. >> - Calling `end()` and then `close()`, although uncommon, will also support >> idempotency and that `close()` call will be a no-op. >> - However, calling `close()` and then `end()` will not guarantee idempotency >> and depending on the implementing subclass, the `end()` may throw an >> exception. >> >> New tests have been included as part of these changes and they continue to >> pass along with existing tests in tier1, tier2 and tier3. When I had >> originally added these new tests, I hadn't used junit. I can convert them to >> junit if that's preferable. >> >> I'll file a CSR shortly. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Chen's suggestion - improve code comment Please keep open, I'm running some experiments for the proposed alternative and I'll update this PR shortly. - PR Comment: https://git.openjdk.org/jdk/pull/19675#issuecomment-2228073341
Re: [jdk23] RFR: 8335820: java/lang/invoke/LFCaching/LFSingleThreadCachingTest.java fails due to IllegalArgumentException: hash must be nonzero
On Mon, 15 Jul 2024 05:53:12 GMT, Adam Sotona wrote: > 8335820: java/lang/invoke/LFCaching/LFSingleThreadCachingTest.java fails due > to IllegalArgumentException: hash must be nonzero This is a clean backport of a P3 bug. Looks good to me for JDK 23. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20180#pullrequestreview-2177064232
Re: RFR: 8303884: jlink --add-options plugin does not allow GNU style options to be provided
On Tue, 2 Jul 2024 12:20:17 GMT, Yasumasa Suenaga wrote: > We cannot pass GNU style options like `--enable-preview` to `jlink > --add-option`. It is hard to use for complex application. > > We have workaround for this issue (see JBS), but I think it is better to fix > on JDK side. Hello @YaSuenag, I haven't had a chance to build your change locally and try it myself, but I suspect this change isn't enough to address the issue. Does this change allow for: jlink ... --add-options --add-exports java.base/jdk.internal.misc=ALL-UNNAMED to work correctly and have the `--add-exports java.base/jdk.internal.misc=ALL-UNNAMED` be considered as the option value for `--add-options`. Additionally, do the current tests pass with your proposed change? - PR Comment: https://git.openjdk.org/jdk/pull/19987#issuecomment-2227372098
Re: RFR: 8315034 : File.mkdirs() occasionally fails to create folders on Windows shared folder
On Fri, 3 Nov 2023 18:11:10 GMT, Weibing Xiao wrote: > File.mkdirs() occasionally fails to create folders on Windows shared folders. > It turned out that Windows API FindFirstFileW created the error > ERROR_NO_MORE_FILES. In some of the cases with a valid file path, this error > still returns this error code, which supposedly should not. > > Adding this error code into the method of lastErrorReportable in the native > code will be handled by JDK. > > To test the fix, it needs to run three Java processes to create the folders > on a remote file server. Hello Olivier, this has been backported to 11, 17 and 21 update releases but not to JDK 8. I don't closely follow the update releases, so I don't know if there's a reason for that. - PR Comment: https://git.openjdk.org/jdk/pull/16502#issuecomment-2227348410
Re: RFR: 8334057: JLinkReproducibleTest.java support receive test.tool.vm.opts [v2]
On Fri, 12 Jul 2024 07:39:11 GMT, SendaoYan wrote: >> Hi all, >> Currently, the testcase `test/jdk/tools/jlink/JLinkReproducibleTest.java` >> doesn't receive jvm options from jtreg. >> I think it's necessory to receive jvm options from jtreg. >> Fix solution similar to >> [JDK-8157850](https://bugs.openjdk.org/browse/JDK-8157850), the change has >> been verified, only change the testacase, the risk is low. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > make variable TOOL_VM_OPTIONS to private The changes look good to me and the testing shows no related failures. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19669#pullrequestreview-2176039655
RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out
Can I please get a review of this test-only change which proposes to fix the test timeout reported in https://bugs.openjdk.org/browse/JDK-8334167? The JBS issue has comments which explains what causes the timeout. The commit in this PR addresses those issues by updating the test specific `ClassFileTransformer` to only instrument application specific class instead of all (core) classes. The test was introduced several years back to verify the feature introduced in https://bugs.openjdk.org/browse/JDK-6263319. As such, the proposed changes in this PR continue to test that feature - we now merely use application specific class' native method to verify the semantics of that feature. Additional cleanups have been done in the test to make sure that if any exception does occur in the ClassFileTransformer then it does get recorded and that then causes the test to fail. With this change, I have run tier1 through tier6 and the test passes. Additionally, without this change I've run the test with a test repeat of 100 with virtual threads enabled and the test hangs occasionally (as expected). With this proposed fix, I have then run the test with virtual threads, around 300 times and it hasn't failed or hung in any of those instances. - Commit messages: - 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out Changes: https://git.openjdk.org/jdk/pull/20154/files Webrev: https://webrevs.openjdk.org/?repo=jdk=20154=00 Issue: https://bugs.openjdk.org/browse/JDK-8334167 Stats: 152 lines in 3 files changed: 74 ins; 37 del; 41 mod Patch: https://git.openjdk.org/jdk/pull/20154.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20154/head:pull/20154 PR: https://git.openjdk.org/jdk/pull/20154
Re: RFR: 8334394: Race condition in Class::protectionDomain [v2]
On Mon, 17 Jun 2024 15:24:27 GMT, Weijun Wang wrote: >> Make sure `pd` is always the same object when `getProtectionDomain0` is null. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > explain why the test is related to the fix Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19752#pullrequestreview-2174075188
Re: RFR: 8334057: JLinkReproducibleTest.java support receive test.tool.vm.opts
On Wed, 12 Jun 2024 02:00:41 GMT, SendaoYan wrote:
> Hi all,
> Currently, the testcase `test/jdk/tools/jlink/JLinkReproducibleTest.java`
> doesn't receive jvm options from jtreg.
> I think it's necessory to receive jvm options from jtreg.
> Fix solution similar to
> [JDK-8157850](https://bugs.openjdk.org/browse/JDK-8157850), the change has
> been verified, only change the testacase, the risk is low.
I'll run this change against our CI instance just to be sure this doesn't cause
unexpected issues. I'll approve the PR once those runs complete.
Thank you for updating the JBS issue description. It now states:
> we found that the test case would start a new java process during the test,
> but the started java child process did not receive the jvm options configured
> from the parent process, resulting in the failure of the fix verification to
> continue normally. Therefore, it is necessary to pass in the jvm options
> configured from the parent process when starting the child java process.
What you propose in this PR looks fine to me and matches some other tests which
do a similar thing. Maybe we should do the same thing in some other tests in
this directory, to keep them consistent. For now though, I think what you have
here is fine and I don't expect you to update these other places.
test/jdk/tools/jlink/JLinkReproducibleTest.java line 41:
> 39: public class JLinkReproducibleTest {
> 40:
> 41: static final String TOOL_VM_OPTIONS =
> System.getProperty("test.tool.vm.opts", "");
Nit - this can be made `private`
-
PR Review: https://git.openjdk.org/jdk/pull/19669#pullrequestreview-2174025942
PR Comment: https://git.openjdk.org/jdk/pull/19669#issuecomment-2224957014
PR Review Comment: https://git.openjdk.org/jdk/pull/19669#discussion_r1675413193
Re: RFR: 8336239: Fix javadoc markup in java.lang.Process
On Thu, 11 Jul 2024 09:36:02 GMT, Pavel Rappo wrote: > Was reading Process' documentation the other day and spotted a markup typo. > Please review this utmost trivial fix. Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20133#pullrequestreview-2171446396
Re: RFR: 8334057: JLinkReproducibleTest.java support receive test.tool.vm.opts
On Wed, 10 Jul 2024 06:12:56 GMT, SendaoYan wrote: >> Hi all, >> Currently, the testcase `test/jdk/tools/jlink/JLinkReproducibleTest.java` >> doesn't receive jvm options from jtreg. >> I think it's necessory to receive jvm options from jtreg. >> The change has been verified, only change the testacase, the risk is low. > > Hi, can anyone take look this PR. Hello @sendaoYan, can you add some details to the JBS issue explaining why this change is necessary and what fails (if anything) in the absence of this change? I suspect you might be running into some failure with this test, but in its current form in the JBS description, it's not clear what the issue is. - PR Comment: https://git.openjdk.org/jdk/pull/19669#issuecomment-083909
Re: [jdk23] RFR: 8335637: Add explicit non-null return value expectations to Object.toString()
On Wed, 10 Jul 2024 21:35:55 GMT, Joe Darcy wrote: > 8335637: Add explicit non-null return value expectations to Object.toString() This is a clean backport of a doc-only change with an approved CSR. Looks good to me for 23. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20124#pullrequestreview-2170701403
Re: RFR: 8335637: Add explicit non-null return value expectations to Object.toString() [v4]
On Wed, 10 Jul 2024 05:02:36 GMT, Joe Darcy wrote:
>> Make well-behaved implementation expectations of Object.{toString, hashCode}
>> explicit.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Narrow scope of the change.
The proposed reduction of the scope of the change to just note that
`toString()` should return a non-null value looks good to me.
-
Marked as reviewed by jpai (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20063#pullrequestreview-2168038159
Re: RFR: 8335637: Add explicit well-behaved expectations to Object.{toString, hashCode}
On Sun, 7 Jul 2024 20:20:30 GMT, Joe Darcy wrote:
> Make well-behaved implementation expectations of Object.{toString, hashCode}
> explicit.
src/java.base/share/classes/java/lang/Object.java line 101:
> 99: * implementation should not use excessive memory or time for its
> 100: * computations and should return a result for cyclic data
> 101: * structures.
Hello Joe, adding this text to set the expectations looks reasonable. However,
I think the text "should return a result for cyclic data structures." feels a
bit odd. If I understand correctly what it's trying to state is that for a
class of the form:
class Foo {
Bar bar;
Foo self;
public void setSelf() {
this.self = this;
}
}
then:
Foo foo = new Foo();
foo.toString(); // or foo.hashCode()
should be able to return the output from hashCode() and toString() even when an
instance of `Foo` has a field which holds the same `Foo` instance. i.e.
`toString()` and `hashCode()` should be able to handle re-entrancy and
shouldn't consume excessive memory or time.
If that understanding is correct, then maybe we could improve that text to
state it differently instead of cyclic data structures? I checked the JDK repo
to see if this term has previously been used but it hasn't. Maybe we should
just state that the hashCode() toString() methods should be able to deal with
re-entrancy?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20063#discussion_r1668312734
Re: RFR: 8328995: Launcher can't open jar files where the offset of the manifest is >4GB [v6]
On Thu, 30 May 2024 20:26:38 GMT, Liam Miller-Cushon wrote: >> Liam Miller-Cushon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Move test to test/jdk/tools/launcher > > @bridgekeeper please keep this open Hello Liam @cushon, please keep this open. Some of us have this on our list to review. - PR Comment: https://git.openjdk.org/jdk/pull/18479#issuecomment-2205652636
Re: RFR: 8301341: LinkedTransferQueue does not respect timeout for poll() [v10]
On Fri, 21 Jul 2023 16:42:25 GMT, Doug Lea wrote: >> This update addresses performance issues across both LinkedTransferQueue and >> SynchronousQueue by creating a common basis for implementation across them >> (mainly in LinkedTransferQueue). Pasting from internal doc summary of >> changes: >> * * Class DualNode replaces Qnode, with fields and methods >> * that apply to any match-based dual data structure, and now >> * usable in other j.u.c classes. in particular, SynchronousQueue. >> * * Blocking control (in class DualNode) accommodates >> * VirtualThreads and (perhaps virtualized) uniprocessors. >> * * All fields of this class (LinkedTransferQueue) are >> * default-initializable (to null), allowing further extension >> * (in particular, SynchronousQueue.Transferer) >> * * Head and tail fields are lazily initialized rather than set >> * to a dummy node, while also reducing retries under heavy >> * contention and misorderings, and relaxing some accesses, >> * requiring accommodation in many places (as well as >> * adjustments in WhiteBox tests). > > Doug Lea 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 13 additional commits since > the last revision: > > - Merge branch 'openjdk:master' into JDK-8301341 > - Address more review comments > - Address review comments > - nitpicks > - Merge branch 'openjdk:master' into JDK-8301341 > - Accommodate white-box tests; use consistent constructions; minor > improvements > - Merge branch 'openjdk:master' into JDK-8301341 > - Simplify contention handling; fix test > - Fix inverted test assert; improve internal documentation; simplify code > - Merge branch 'openjdk:master' into JDK-8301341 > - ... and 3 more: https://git.openjdk.org/jdk/compare/37ac993c...f53cee67 Like Doug notes, the JDK backports are managed as a separate project. Details are available here https://openjdk.org/projects/jdk-updates/. For JDK 17 the corresponding project is https://wiki.openjdk.org/display/JDKUpdates/JDK+17u which has the necessary details about the process as well as the mailing list details where you can bring up the backporting question. - PR Comment: https://git.openjdk.org/jdk/pull/14317#issuecomment-2204851286
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes [v5]
On Sat, 29 Jun 2024 18:24:46 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which suggests we rename `ZipEntry.extraAttributes` >> to `ZipEntry.externalFileAttributes`. >> >> This field was introduced in >> [JDK-8218021](https://bugs.openjdk.org/browse/JDK-8218021), originally under >> the name `ZipEntry.posixPerms`. >> [JDK-8250968](https://bugs.openjdk.org/browse/JDK-8250968) later renamed the >> field to `ZipEntry.extraAttributes` and extended its semantics to hold the >> full two-byte value of the `external file attributes` field, as defined by >> `APPNOTE.TXT` >> >> The name `extraAttributes` is misleading. It has nothing to do with the >> `extra field` (an unrelated structure defined in `APPNOTE.TXT`), although >> the name indicates it does. >> >> To prevent confusion and make life easier for future maintainers, I suggest >> we rename this field to `ZipEntry.externalFileAttributes` and update related >> methods, parameters and comments accordingly. >> >> While this change is a straightforward renaming, reviewers should consider >> whether it carries its weight, especially considering it might complicate >> future backports. >> >> As a note to reviewers, this PR includes the following intended updates: >> >> - Rename `ZipEntry.extraAttributes` and any references to this field to >> `ZipEntry.externalFileAttributes` >> - Update `JavaUtilZipFileAccess` to similarly rename methods to >> `setExternalFileAttributes` and `getExternalFileAttributes` >> - Rename the field `JarSigner.extraAttrsDetected` to >> `JarSigner.externalFileAttrsDetected` >> - Rename a local variable in `JarSigner.writeEntry` to >> `externalFileAttributes` >> - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to >> `externalFileAttributesDetected` >> - Rename resource string key names in `s.s.t.jarsigner.Resources` from >> `extra.attributes.detected` to `external.file.attributes.detected` >> - Rename method `SymlinkTest.verifyExtraAttrs` to >> `verifyExternalFileAttributes`, also updated two references to 'extra >> attributes' in this method >> - Updated copyright in all affected files >> >> If the resource file changes should be dropped and instead handled via >> `msgdop` updates, let me know and I can revert the non-default files. >> >> I did a search across the code base to find 'extraAttrs', 'extra.attr' after >> these updates, and found nothing related to zip/jar. The `zip` and `jar` >> tests run clean: >> >> >> make test TEST="test/jdk/java/util/jar" >> make test TEST="test/jdk/java/util/zip" > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge branch 'master' into zipentry-external-attributes > - Rename SymlinkTest.verifyExternalAttrs to verifyExternalFileAttributes > - Rename ZipFileSystem.Entry.posixPerms to externalFileAttributes > - For clarity, use "externalFileAttributes" instead of "externalAttributes" > - Merge branch 'master' into zipentry-external-attributes > - Update copyright years for 2024 > - Merge branch 'master' into zipentry-external-attributes > - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes Marked as reviewed by jpai (Reviewer). Hello Eirik, the latest changes in this PR (`292d6801`) look good to me. However, these changes cause some (expected) compilation failures in some of the internal classes within some Oracle specific JDK classes. Those tests aren't accessible for external contributors. I will be updating that code to address those failures. That would mean that the integration of this PR will have to be co-ordinated. Can you issue a `/integrate delegate` command on this PR so that it then allows me to do the actual integration along with the Oracle side of changes? - PR Review: https://git.openjdk.org/jdk/pull/16952#pullrequestreview-2153714037 PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-2203081198
Re: RFR: 8335060: ClassCastException after JDK-8294960
On Wed, 26 Jun 2024 06:53:28 GMT, Adam Sotona wrote: > Conversion of `java.lang.invoke` package to Class-File API is failing to > execute method handles with specific type conversion requirements. Root cause > is in the new `TypeConvertingMethodAdapter::primitiveTypeKindFromClass` > implementation. Original code has been matching the types by hash code and it > mistakenly appeared to be matching the primitive types. > > This patch fixes `TypeConvertingMethodAdapter::primitiveTypeKindFromClass` > and adds tests to better cover `TypeConvertingMethodAdapter` functionality. > > Please review. > > Thanks, > Adam Hello Adam, I'm not familiar with this code. But looking at the call sites of this method and the code in and around those call sites (including code comments), this change looks OK to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19898#pullrequestreview-2151108992
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalFileAttributes [v5]
On Sat, 29 Jun 2024 18:24:46 GMT, Eirik Bjørsnøs wrote: >> Please consider this PR which suggests we rename `ZipEntry.extraAttributes` >> to `ZipEntry.externalFileAttributes`. >> >> This field was introduced in >> [JDK-8218021](https://bugs.openjdk.org/browse/JDK-8218021), originally under >> the name `ZipEntry.posixPerms`. >> [JDK-8250968](https://bugs.openjdk.org/browse/JDK-8250968) later renamed the >> field to `ZipEntry.extraAttributes` and extended its semantics to hold the >> full two-byte value of the `external file attributes` field, as defined by >> `APPNOTE.TXT` >> >> The name `extraAttributes` is misleading. It has nothing to do with the >> `extra field` (an unrelated structure defined in `APPNOTE.TXT`), although >> the name indicates it does. >> >> To prevent confusion and make life easier for future maintainers, I suggest >> we rename this field to `ZipEntry.externalFileAttributes` and update related >> methods, parameters and comments accordingly. >> >> While this change is a straightforward renaming, reviewers should consider >> whether it carries its weight, especially considering it might complicate >> future backports. >> >> As a note to reviewers, this PR includes the following intended updates: >> >> - Rename `ZipEntry.extraAttributes` and any references to this field to >> `ZipEntry.externalFileAttributes` >> - Update `JavaUtilZipFileAccess` to similarly rename methods to >> `setExternalFileAttributes` and `getExternalFileAttributes` >> - Rename the field `JarSigner.extraAttrsDetected` to >> `JarSigner.externalFileAttrsDetected` >> - Rename a local variable in `JarSigner.writeEntry` to >> `externalFileAttributes` >> - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to >> `externalFileAttributesDetected` >> - Rename resource string key names in `s.s.t.jarsigner.Resources` from >> `extra.attributes.detected` to `external.file.attributes.detected` >> - Rename method `SymlinkTest.verifyExtraAttrs` to >> `verifyExternalFileAttributes`, also updated two references to 'extra >> attributes' in this method >> - Updated copyright in all affected files >> >> If the resource file changes should be dropped and instead handled via >> `msgdop` updates, let me know and I can revert the non-default files. >> >> I did a search across the code base to find 'extraAttrs', 'extra.attr' after >> these updates, and found nothing related to zip/jar. The `zip` and `jar` >> tests run clean: >> >> >> make test TEST="test/jdk/java/util/jar" >> make test TEST="test/jdk/java/util/zip" > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - Merge branch 'master' into zipentry-external-attributes > - Rename SymlinkTest.verifyExternalAttrs to verifyExternalFileAttributes > - Rename ZipFileSystem.Entry.posixPerms to externalFileAttributes > - For clarity, use "externalFileAttributes" instead of "externalAttributes" > - Merge branch 'master' into zipentry-external-attributes > - Update copyright years for 2024 > - Merge branch 'master' into zipentry-external-attributes > - Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes Hello Eirik, I'll run some tests and review this PR this coming week. - PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-2198432408
Re: RFR: 8334810: Redo: Un-ProblemList LocaleProvidersRun and CalendarDataRegression
On Mon, 24 Jun 2024 04:25:45 GMT, Yude Lin wrote: > [JDK-8318107](https://bugs.openjdk.org/browse/JDK-8318107) Un-ProblemListed > LocaleProvidersRun and CalendarDataRegression, and > [JDK-8288899](https://bugs.openjdk.org/browse/JDK-8288899) added them back. > I'm guessing it's a mistake in resolving merge conflict. Hello Yude, can you do `/issue add 8268379` so that even 8268379 gets resolved when this PR gets integrated? Otherwise, 8268379 will still remain open after removing the tests from the problem listing. - PR Comment: https://git.openjdk.org/jdk/pull/19849#issuecomment-2188111502
Re: [jdk23] RFR: 8334708: FFM: two javadoc problems
On Sun, 23 Jun 2024 06:32:50 GMT, Hannes Greule wrote: > Hi all, > > This pull request contains a backport of commit > [72ca7baf](https://github.com/openjdk/jdk/commit/72ca7bafcd49a98c1fe09da72e4e47683f052e9d) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Hannes Greule on 22 Jun 2024 and > was reviewed by Maurizio Cimadamore. > > Thanks! This doc-only clean backport looks fine to me for 23. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19846#pullrequestreview-2134273139
Re: RFR: 8331552: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Testing: _tier1-tier5 pending..._ Hello Christian, it would be better to merge latest master branch into this PR before integrating this. It looks like it currently uses `master` from more than a month back. - PR Comment: https://git.openjdk.org/jdk/pull/19052#issuecomment-2171466397
Re: RFR: 8331552: Update to use jtreg 7.4
On Thu, 2 May 2024 09:48:51 GMT, Christian Stein wrote: > Please review the change to update to using `jtreg` **7.4**. > > The primary change is to the `jib-profiles.js` file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > Testing: _tier1-tier5 pending..._ Looks OK to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19052#pullrequestreview-2121354181
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v3]
> Can I please get a review of this enhancement which proposes to enhance > `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`? > > The actual work for this was done a few years back when we discussed the > proposed approaches and then I raised a RFR. At that time I couldn't take > this to completion. The current changes in this PR involve the implementation > that was discussed at that time and also have implemented the review > suggestions from that time. Here are those previous discussions and reviews: > > https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html > https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html > https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html > > To summarize those discussions, we had concluded that: > - `Deflater` and `Inflater` will implement the `AutoCloseable` interface > - In the `close()` implementation we will invoke the `end()` method (`end()` > can be potentially overridden by subclasses). > - `close()` will be specified and implemented to be idempotent. Calling > `close()` a second time or more will be a no-op. > - Calling `end()` and then `close()`, although uncommon, will also support > idempotency and that `close()` call will be a no-op. > - However, calling `close()` and then `end()` will not guarantee idempotency > and depending on the implementing subclass, the `end()` may throw an > exception. > > New tests have been included as part of these changes and they continue to > pass along with existing tests in tier1, tier2 and tier3. When I had > originally added these new tests, I hadn't used junit. I can convert them to > junit if that's preferable. > > I'll file a CSR shortly. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Chen's suggestion - improve code comment - Changes: - all: https://git.openjdk.org/jdk/pull/19675/files - new: https://git.openjdk.org/jdk/pull/19675/files/79e68e91..4a52fe2b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19675=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19675=01-02 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19675/head:pull/19675 PR: https://git.openjdk.org/jdk/pull/19675
Integrated: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher
On Thu, 6 Jun 2024 11:53:10 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to remove the > `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? > > This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that > JBS issue, in a recent PR discussion, it was suggested > https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this > macro should be removed and the failure of a JNI specified operation (the > ones for which this macro is being used) should be determined based on a > `NULL` value returned from that function. The commit in this PR removes this > macros and updates the call sites to do a `NULL` check. > > Given the nature of this change, no new tests have been added. tier1, tier2 > and tier3 testing passed successfully with these changes. This pull request has now been integrated. Changeset: efab48c0 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/efab48c06554476eae7a7bd946dee033d16a9c38 Stats: 68 lines in 1 file changed: 37 ins; 9 del; 22 mod 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/19576
Re: RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher [v2]
On Thu, 6 Jun 2024 13:28:55 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change which proposes to remove the >> `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? >> >> This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that >> JBS issue, in a recent PR discussion, it was suggested >> https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this >> macro should be removed and the failure of a JNI specified operation (the >> ones for which this macro is being used) should be determined based on a >> `NULL` value returned from that function. The commit in this PR removes this >> macros and updates the call sites to do a `NULL` check. >> >> Given the nature of this change, no new tests have been added. tier1, tier2 >> and tier3 testing passed successfully with these changes. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > improve comments Thank you for the review Alan. I'll integrate this in a few hours. - PR Comment: https://git.openjdk.org/jdk/pull/19576#issuecomment-2167053874
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v2]
On Thu, 13 Jun 2024 16:02:38 GMT, Chen Liang wrote: > My suggestion is that our comment should say why we have this check here even > though there's already an identical check in end(). I now see what you mean. I'll update this appropriately tomorrow. - PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1638559184
Re: RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher [v2]
On Thu, 6 Jun 2024 13:28:55 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change which proposes to remove the >> `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? >> >> This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that >> JBS issue, in a recent PR discussion, it was suggested >> https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this >> macro should be removed and the failure of a JNI specified operation (the >> ones for which this macro is being used) should be determined based on a >> `NULL` value returned from that function. The commit in this PR removes this >> macros and updates the call sites to do a `NULL` check. >> >> Given the nature of this change, no new tests have been added. tier1, tier2 >> and tier3 testing passed successfully with these changes. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > improve comments Any reviews for this please? - PR Comment: https://git.openjdk.org/jdk/pull/19576#issuecomment-2165803780
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v2]
On Wed, 12 Jun 2024 21:07:25 GMT, Chen Liang wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> convert the tests to junit
>
> src/java.base/share/classes/java/util/zip/Deflater.java line 904:
>
>> 902: public void close() {
>> 903: synchronized (zsRef) {
>> 904: // check if already closed
>
> Should we comment `// in case subclasses override the closed check in end()`
> instead? Otherwise the duplicate comments and checks are confusing.
Hello Chen, we want the close() to be idempotent irrespective of whether or not
end() is overridden. That check in close() and the code comment on that line is
to indicate that. If the comment is adding to confusion, I can remove it - I
don't think it's a must to have it to understand the check.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1638288513
Re: RFR: 8334162: Gatherer.defaultCombiner has an erronous @see-link [v2]
On Thu, 13 Jun 2024 10:48:15 GMT, Viktor Klang wrote: >> Erronous Javadoc reference addressed. This should be backported to 23. > > Viktor Klang has updated the pull request incrementally with two additional > commits since the last revision: > > - Tweaking the copyright year to reflect initial year AND current > - Updating copyright year The copyright year update to `2023, 2024` looks fine to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19682#pullrequestreview-2115439115
Re: RFR: 8333358: java/io/IO/IO.java test fails intermittently [v3]
On Thu, 13 Jun 2024 08:05:15 GMT, Alan Bateman wrote: >> Looking briefly at this, I think we should be able to provide this as a >> feature of jtreg's support of `junit` test itself. I've created >> https://bugs.openjdk.org/browse/CODETOOLS-7903751 to investigate this >> support in jtreg. > > This is something that I discussed with Christen Stein about. We've tossed > around some ideas, he created CODETOOLS-7903752 so we may have a dup. I've closed mine as the duplicate of the other. - PR Review Comment: https://git.openjdk.org/jdk/pull/19627#discussion_r1637778998
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v2]
On Wed, 12 Jun 2024 21:08:35 GMT, Chen Liang wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> convert the tests to junit > > test/jdk/java/util/zip/DeflaterClose.java line 2: > >> 1: /* >> 2: * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved. > > Suggestion: > > * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. > > Same for `InflaterClose.java`. Hello Chen, these tests were actually introduced in a RFR back in 2019, so the original year is correct. I've now converted these tests to junit and updated the PR and updated the copyright years as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/19675#discussion_r1637667063
Re: RFR: 8225763: Inflater and Deflater should implement AutoCloseable [v2]
> Can I please get a review of this enhancement which proposes to enhance > `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`? > > The actual work for this was done a few years back when we discussed the > proposed approaches and then I raised a RFR. At that time I couldn't take > this to completion. The current changes in this PR involve the implementation > that was discussed at that time and also have implemented the review > suggestions from that time. Here are those previous discussions and reviews: > > https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html > https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html > https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html > > To summarize those discussions, we had concluded that: > - `Deflater` and `Inflater` will implement the `AutoCloseable` interface > - In the `close()` implementation we will invoke the `end()` method (`end()` > can be potentially overridden by subclasses). > - `close()` will be specified and implemented to be idempotent. Calling > `close()` a second time or more will be a no-op. > - Calling `end()` and then `close()`, although uncommon, will also support > idempotency and that `close()` call will be a no-op. > - However, calling `close()` and then `end()` will not guarantee idempotency > and depending on the implementing subclass, the `end()` may throw an > exception. > > New tests have been included as part of these changes and they continue to > pass along with existing tests in tier1, tier2 and tier3. When I had > originally added these new tests, I hadn't used junit. I can convert them to > junit if that's preferable. > > I'll file a CSR shortly. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: convert the tests to junit - Changes: - all: https://git.openjdk.org/jdk/pull/19675/files - new: https://git.openjdk.org/jdk/pull/19675/files/6e32965d..79e68e91 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19675=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19675=00-01 Stats: 406 lines in 2 files changed: 64 ins; 99 del; 243 mod Patch: https://git.openjdk.org/jdk/pull/19675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19675/head:pull/19675 PR: https://git.openjdk.org/jdk/pull/19675
Re: RFR: 8333358: java/io/IO/IO.java test fails intermittently [v3]
On Thu, 13 Jun 2024 06:29:36 GMT, Alan Bateman wrote: >> Pavel Rappo has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Disable timeout in expect scripts > > test/jdk/java/io/IO/IO.java line 192: > >> 190: // adapted from >> https://junit.org/junit5/docs/current/user-guide/#extensions-lifecycle-callbacks-timing-extension >> 191: >> 192: public static class TimingExtension implements >> BeforeTestExecutionCallback, > > In passing, this feels like infrastructure that we could use in several areas > as it regularly comes up as to how long specific tests take (the "seconds" > line recorded by jtreg in the .jtr file is too coarse grain when there are > many tests in the same source file). Looking briefly at this, I think we should be able to provide this as a feature of jtreg's support of `junit` test itself. I've created https://bugs.openjdk.org/browse/CODETOOLS-7903751 to investigate this support in jtreg. - PR Review Comment: https://git.openjdk.org/jdk/pull/19627#discussion_r1637663845
Re: RFR: 8333358: java/io/IO/IO.java test fails intermittently [v2]
On Wed, 12 Jun 2024 19:02:30 GMT, Pavel Rappo wrote: > Preliminary testing on some affected configurations showed that even 20 > seconds might not be enough. Initially, I suggested that it would be hard to > imagine, yet here we are. I haven't seen the jtreg launch command or the jtr files for these timeout failures, but in the past on systems that Matthias has run into similar issues, it was observed that those systems run with a very high `-concurrency` value (which implies too many test processes running at the same time) which could cause slowness. I don't know if that's what is a contributing factor here. > A good solution would be to disable expect timeout and, thus, fall back onto > jtreg timeout. I think that's a better solution. That removes the part where we have to guess what's the best/appropriate timeout value to use in the test. - PR Comment: https://git.openjdk.org/jdk/pull/19627#issuecomment-2164662278
Re: RFR: 8334162: Gatherer.defaultCombiner has an erronous @see-link
On Wed, 12 Jun 2024 20:58:28 GMT, Viktor Klang wrote: > Erronous Javadoc reference addressed. This should be backported to 23. Hello Viktor, the change looks fine to me. The copyright year on the file will need an update before integrating. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19682#pullrequestreview-2114507628
RFR: 8225763: Inflater and Deflater should implement AutoCloseable
Can I please get a review of this enhancement which proposes to enhance `java.util.zip.Deflater/Inflater` classes to now implement `AutoCloseable`? The actual work for this was done a few years back when we discussed the proposed approaches and then I raised a RFR. At that time I couldn't take this to completion. The current changes in this PR involve the implementation that was discussed at that time and also have implemented the review suggestions from that time. Here are those previous discussions and reviews: https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html To summarize those discussions, we had concluded that: - `Deflater` and `Inflater` will implement the `AutoCloseable` interface - In the `close()` implementation we will invoke the `end()` method (`end()` can be potentially overridden by subclasses). - `close()` will be specified and implemented to be idempotent. Calling `close()` a second time or more will be a no-op. - Calling `end()` and then `close()`, although uncommon, will also support idempotency and that `close()` call will be a no-op. - However, calling `close()` and then `end()` will not guarantee idempotency and depending on the implementing subclass, the `end()` may throw an exception. New tests have been included as part of these changes and they continue to pass along with existing tests in tier1, tier2 and tier3. When I had originally added these new tests, I hadn't used junit. I can convert them to junit if that's preferable. I'll file a CSR shortly. - Commit messages: - fix whitespace - 8225763: Inflater and Deflater should implement AutoCloseable Changes: https://git.openjdk.org/jdk/pull/19675/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19675=00 Issue: https://bugs.openjdk.org/browse/JDK-8225763 Stats: 724 lines in 6 files changed: 673 ins; 9 del; 42 mod Patch: https://git.openjdk.org/jdk/pull/19675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19675/head:pull/19675 PR: https://git.openjdk.org/jdk/pull/19675
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Thu, 22 Feb 2024 05:40:10 GMT, Joe Darcy wrote: >> Update: confirmed that the new test fails without the fix. > >> Update: confirmed that the new test fails without the fix. > > Thanks for verifying the test checks the fix; I'll let others who have worked > more directly on the random code review the actual fix. > @jddarcy Added a regression test, and currently working on adjusting it (see > https://github.com/Pr0methean/jdk/actions/runs/798127) to ensure we have > a case that fails without the fix, passes with the fix, and is practical to > run within the usual unit-test timeouts. I gave this a try locally. It doesn't fail for me without the source code changes proposed in this PR. I see the following output from the test without the source code changes: got 1.0 for max 1.0 got 2.0 for max 2.0 got 3.0 for max 3.0 got 4.0 for max 4.0 got 5.0 for max 5.0 got 6.0 for max 6.0 got 7.0 for max 7.0 got 11.353912041222094 for max 8.0 got 11.353912041222094 for max 9.0 With the proposed changes in this PR, the test continues to pass and I see this output: got 7.569274694148063 for max 1.0 got 7.569274694148063 for max 2.0 got 7.569274694148063 for max 3.0 got 7.569274694148063 for max 4.0 got 7.569274694148063 for max 5.0 got 7.569274694148063 for max 6.0 got 7.569274694148063 for max 7.0 got 11.353912041222094 for max 8.0 got 11.353912041222094 for max 9.0 - PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2162623391
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick wrote:
>> This provides a slightly more accurate bounding limit for
>> `computeNextExponentialSoftCapped` when calling it from
>> `computeNextGaussian`. This could cause the `while
>> (computeNextExponentialSoftCapped(rng, limit) < limit)` check in
>> `computeNextGaussian` on line 1402 to always be true, making the
>> `nextGaussian` runtime unbounded in the worst case; but more likely, it
>> would give a result that was truncated too close to zero.
>>
>> This change is being tested prior to submission to OpenJDK by
>> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf.
>
> Chris Hennick has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Bug fix: add-exports was for wrong package
test/jdk/jdk/internal/util/random/RandomSupportTest.java line 32:
> 30: for (double max = 1.0; max < 10.0; max++) {
> 31: WorstCaseRandomGenerator rng = new WorstCaseRandomGenerator();
> 32:
> assertTrue(RandomSupport.computeNextExponentialSoftCapped(rng, max) >= max);
Would you mind changing this to something like:
double val = RandomSupport.computeNextExponentialSoftCapped(rng, max);
assertTrue(val >= max, val + " isn't >= " + max);
That way if this test fails for any reason, then we get the necessary details
on what the computed value is.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17703#discussion_r1636139162
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick wrote: >> This provides a slightly more accurate bounding limit for >> `computeNextExponentialSoftCapped` when calling it from >> `computeNextGaussian`. This could cause the `while >> (computeNextExponentialSoftCapped(rng, limit) < limit)` check in >> `computeNextGaussian` on line 1402 to always be true, making the >> `nextGaussian` runtime unbounded in the worst case; but more likely, it >> would give a result that was truncated too close to zero. >> >> This change is being tested prior to submission to OpenJDK by >> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf. > > Chris Hennick has updated the pull request incrementally with one additional > commit since the last revision: > > Bug fix: add-exports was for wrong package test/jdk/jdk/internal/util/random/RandomSupportTest.java line 5: > 3: * @bug 8326227 > 4: * @modules java.base/jdk.internal.util.random > 5: * @summary Verify that RandomSupport methods behave as specified Nit - can you move this `@summary` line after the `@bug` line? It would then match the recommended tag order https://openjdk.org/jtreg/tag-spec.html#ORDER - PR Review Comment: https://git.openjdk.org/jdk/pull/17703#discussion_r1636122002
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick wrote:
>> This provides a slightly more accurate bounding limit for
>> `computeNextExponentialSoftCapped` when calling it from
>> `computeNextGaussian`. This could cause the `while
>> (computeNextExponentialSoftCapped(rng, limit) < limit)` check in
>> `computeNextGaussian` on line 1402 to always be true, making the
>> `nextGaussian` runtime unbounded in the worst case; but more likely, it
>> would give a result that was truncated too close to zero.
>>
>> This change is being tested prior to submission to OpenJDK by
>> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf.
>
> Chris Hennick has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Bug fix: add-exports was for wrong package
src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
1214:
> 1212: // We didn't use the upper part of U1 after all. We'll
> probably be able to use it later.
> 1213: if (maxValue <= DoubleZigguratTables.exponentialX0) {
> 1214: return DoubleZigguratTables.exponentialX0;
Chris, could you explain why this change to this if block is necessary? Guy
notes that (I've paraphrased):
> I can see that (without this proposed change here), if `maxValue` is not
> greater than `DoubleZigguratTables.exponentialX0`, then the subsequent
> computation for `maxExtraMinus1`, a few lines below using `Math.round()`,
> will compute 1 or 2 for the value of `maxExtraMinus1`. But with this proposed
> change, it just returns `DoubleZigguratTables.exponentialX0`, which by the
> contract of `computeNextExponentialSoftCapped` is okay (the doc says "{@code
> maxValue} is a "soft" cap in that a value larger than {@code maxValue} may be
> returned in order to save a calculation" and I remember that that is true).
> So that is also okay. The motivation for doing so would be that it saves
> computation time overall. The part I don't quite understand yet is the
> judgment that it will in fact save computation time overall. If you take
> advantage of the "soft cap" then it could cause additional iteration of the
> loop in `computeNextGaussian` where this `computeNextExponentialSoftCapped`
> method gets called. I'm u
ncertain whether the change to this if block is guaranteed or likely to save
computation time.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17703#discussion_r1636024966
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick wrote: >> This provides a slightly more accurate bounding limit for >> `computeNextExponentialSoftCapped` when calling it from >> `computeNextGaussian`. This could cause the `while >> (computeNextExponentialSoftCapped(rng, limit) < limit)` check in >> `computeNextGaussian` on line 1402 to always be true, making the >> `nextGaussian` runtime unbounded in the worst case; but more likely, it >> would give a result that was truncated too close to zero. >> >> This change is being tested prior to submission to OpenJDK by >> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf. > > Chris Hennick has updated the pull request incrementally with one additional > commit since the last revision: > > Bug fix: add-exports was for wrong package src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line 1222: > 1220: // Math.round rounds toward infinity in conversion to long; > adding 1.0 corrects for any > 1221: // downward rounding error in the division > 1222: maxExtraMinus1 = Math.round(1.0 + maxValue / > DoubleZigguratTables.exponentialX0); We had asked Guy Steele for his inputs on these proposed changes. Guy reviewed these changes and for this specific if/else block change, he notes that (I've paraphrased it): > the (proposed new) computation of `maxExtraMinus1` looks okay to me: it’s > always okay for maxExtraMinus1 to be a bit larger than you might expect; the > only downside is that the (for) loop starting (on the next line) might take > extra iterations. - PR Review Comment: https://git.openjdk.org/jdk/pull/17703#discussion_r1636013052
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Wed, 21 Feb 2024 02:18:08 GMT, Chris Hennick wrote: >> This provides a slightly more accurate bounding limit for >> `computeNextExponentialSoftCapped` when calling it from >> `computeNextGaussian`. This could cause the `while >> (computeNextExponentialSoftCapped(rng, limit) < limit)` check in >> `computeNextGaussian` on line 1402 to always be true, making the >> `nextGaussian` runtime unbounded in the worst case; but more likely, it >> would give a result that was truncated too close to zero. >> >> This change is being tested prior to submission to OpenJDK by >> https://github.com/openjdk/jdk/pull/17703/commits/b8be051cbf40a6a05fafc6a2c76942e9e0b11fdf. > > Chris Hennick has updated the pull request incrementally with one additional > commit since the last revision: > > Bug fix: add-exports was for wrong package Hello Chris, I just noticed that there have been (unrelated) changes in this file in the master. Could you please merge the latest master branch changes into this PR? - PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2162345189
Integrated: 8222884: ConcurrentClassDescLookup.java times out intermittently
On Tue, 11 Jun 2024 13:09:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which proposes to address > intermittent failures in > `test/jdk/java/io/Serializable/concurrentClassDescLookup/ConcurrentClassDescLookup.java`? > > This addresses https://bugs.openjdk.org/browse/JDK-8222884. As noted in that > issue, the test in its current form has a potential race. The test main > thread launches 50 "successful lookup" tasks and 50 "failed lookup" tasks. > Each task is a `Thread`. The test passes an `Object` instance shared by 50 of > these tasks and each task when it starts execution in its `run()` method does > a `wait()` on that `Object` instance. The test main thread then after > launching 50 tasks, sleeps for a second (to let the 50 `Thread`s start > execution and arrive at the `Object.wait()`) and then after waking up the > main thread does a `notifyAll()` on that `Object` instance. In theory and > likely in practice too, it's possible that (especially) for the last batch of > 50 tasks (the "failing lookup" tasks), the `notifyAll()` might end up getting > invoked before one or more task Thread(s) have actually done a `wait()`. That > can then mean that one or more task `Thread`(s) will keep `wait()`ing > indefinitely with no on e `notify()`ing the `Object` instance to proceed with the task execution. > > The commit in this PR removes the use of `Object.wait()/notifyAll()` and > instead uses a `CountdownLatch` to allow for a more deterministic barrier. > The test continues to pass with this change. Additionally, this change was > run against a CI setup, by Matthias, where it was previously failing > intermittently. Matthias reports that it hasn't failed after this change. This pull request has now been integrated. Changeset: bd046d9b Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/bd046d9b9e79e4eea89c72af358961ef6e98e660 Stats: 36 lines in 1 file changed: 7 ins; 7 del; 22 mod 8222884: ConcurrentClassDescLookup.java times out intermittently Reviewed-by: rriggs, mbaesken - PR: https://git.openjdk.org/jdk/pull/19659
Re: RFR: 8222884: ConcurrentClassDescLookup.java times out intermittently
On Tue, 11 Jun 2024 13:09:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which proposes to address > intermittent failures in > `test/jdk/java/io/Serializable/concurrentClassDescLookup/ConcurrentClassDescLookup.java`? > > This addresses https://bugs.openjdk.org/browse/JDK-8222884. As noted in that > issue, the test in its current form has a potential race. The test main > thread launches 50 "successful lookup" tasks and 50 "failed lookup" tasks. > Each task is a `Thread`. The test passes an `Object` instance shared by 50 of > these tasks and each task when it starts execution in its `run()` method does > a `wait()` on that `Object` instance. The test main thread then after > launching 50 tasks, sleeps for a second (to let the 50 `Thread`s start > execution and arrive at the `Object.wait()`) and then after waking up the > main thread does a `notifyAll()` on that `Object` instance. In theory and > likely in practice too, it's possible that (especially) for the last batch of > 50 tasks (the "failing lookup" tasks), the `notifyAll()` might end up getting > invoked before one or more task Thread(s) have actually done a `wait()`. That > can then mean that one or more task `Thread`(s) will keep `wait()`ing > indefinitely with no on e `notify()`ing the `Object` instance to proceed with the task execution. > > The commit in this PR removes the use of `Object.wait()/notifyAll()` and > instead uses a `CountdownLatch` to allow for a more deterministic barrier. > The test continues to pass with this change. Additionally, this change was > run against a CI setup, by Matthias, where it was previously failing > intermittently. Matthias reports that it hasn't failed after this change. Thank you Roger and Matthias for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/19659#issuecomment-2162143042
RFR: 8222884: ConcurrentClassDescLookup.java times out intermittently
Can I please get a review of this test-only change which proposes to address intermittent failures in `test/jdk/java/io/Serializable/concurrentClassDescLookup/ConcurrentClassDescLookup.java`? This addresses https://bugs.openjdk.org/browse/JDK-8222884. As noted in that issue, the test in its current form has a potential race. The test main thread launches 50 "successful lookup" tasks and 50 "failed lookup" tasks. Each task is a `Thread`. The test passes an `Object` instance shared by 50 of these tasks and each task when it starts execution in its `run()` method does a `wait()` on that `Object` instance. The test main thread then after launching 50 tasks, sleeps for a second (to let the 50 `Thread`s start execution and arrive at the `Object.wait()`) and then after waking up the main thread does a `notifyAll()` on that `Object` instance. In theory and likely in practice too, it's possible that (especially) for the last batch of 50 tasks (the "failing lookup" tasks), the `notifyAll()` might end up getting invoked before one or more task Thread(s) have actually done a `wait()`. That can then mean that one or more task `Thread`(s) will keep `wait()`ing indefinitely with no one `notify()`ing the `Object` instance to proceed with the task execution. The commit in this PR removes the use of `Object.wait()/notifyAll()` and instead uses a `CountdownLatch` to allow for a more deterministic barrier. The test continues to pass with this change. Additionally, this change was run against a CI setup, by Matthias, where it was previously failing intermittently. Matthias reports that it hasn't failed after this change. - Commit messages: - 8222884: ConcurrentClassDescLookup.java times out intermittently Changes: https://git.openjdk.org/jdk/pull/19659/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19659=00 Issue: https://bugs.openjdk.org/browse/JDK-8222884 Stats: 36 lines in 1 file changed: 7 ins; 7 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19659.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19659/head:pull/19659 PR: https://git.openjdk.org/jdk/pull/19659
Re: RFR: 8173970: jar tool should have a way to extract to a directory [v7]
> Can I please get a review for this patch which proposes to implement the > enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970? > > The commit in this PR introduces the `-o` and `--output-dir` option to the > `jar` command. The option takes a path to a destination directory as a value > and extracts the contents of the jar into that directory. This is an optional > option and the changes in the commit continue to maintain backward > compatibility where the jar is extracted into current directory, if no `-o` > or `--output-dir` option has been specified. > > As far as I know, there hasn't been any discussion on what the name of this > new option should be. I was initially thinking of using `-d` but that is > currently used by the `jar` command for a different purpose. So I decided to > use `-o` and `--output-dir`. This is of course open for change depending on > any suggestions in this PR. > > The commit in this PR also updates the `jar.properties` file which contains > the English version of the jar command's `--help` output. However, no changes > have been done to the internationalization files corresponding to this one > (for example: `jar_de.properties`), because I don't know what process needs > to be followed to have those files updated (if at all they need to be > updated). > > The commit also includes a jtreg testcase which verifies the usage of this > new option. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - merge latest from master branch - merge latest from master branch - cleanup testExtractNoDestDirWithPFlag() test - merge latest from master branch - merge latest from master branch - convert the new test to junit - merge latest from master branch - Lance's review - include tests for --extract long form option - cleanup after each test - Adjust test for new error messages - ... and 7 more: https://git.openjdk.org/jdk/compare/7b43a8cd...2bd13ece - Changes: https://git.openjdk.org/jdk/pull/2752/files Webrev: https://webrevs.openjdk.org/?repo=jdk=2752=06 Stats: 569 lines in 4 files changed: 558 ins; 0 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/2752.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/2752/head:pull/2752 PR: https://git.openjdk.org/jdk/pull/2752
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]
On Thu, 6 Jun 2024 14:02:03 GMT, Jaikiran Pai wrote: >> Can I please get a review of this doc-only change which proposes to improve >> the code snippet that's in `java.util.zip.Deflater` and >> `java.util.zip.Inflater` to better explain the usage of those classes? This >> addresses https://bugs.openjdk.org/browse/JDK-8026127. >> >> The commit in the PR cleans up the snippet to correctly compress/decompress >> till the `Deflater` and `Inflater` are `finished()`. Additionally, the >> snippet also shows that the `Deflater` and `Inflater` are expected to be >> closed when their usage it done, to release the resources held by those >> instances. >> >> I've run `make docs-image` locally to verify that the generated snippet >> content as well as the link from `Inflater` work fine in the rendered >> javadoc HTML. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - minor change to the comment > - move the snippet to an external snippet Thank you Lance for the review. - PR Comment: https://git.openjdk.org/jdk/pull/19507#issuecomment-2153639493
Integrated: 8026127: Deflater/Inflater documentation incomplete/misleading
On Sat, 1 Jun 2024 04:33:57 GMT, Jaikiran Pai wrote: > Can I please get a review of this doc-only change which proposes to improve > the code snippet that's in `java.util.zip.Deflater` and > `java.util.zip.Inflater` to better explain the usage of those classes? This > addresses https://bugs.openjdk.org/browse/JDK-8026127. > > The commit in the PR cleans up the snippet to correctly compress/decompress > till the `Deflater` and `Inflater` are `finished()`. Additionally, the > snippet also shows that the `Deflater` and `Inflater` are expected to be > closed when their usage it done, to release the resources held by those > instances. > > I've run `make docs-image` locally to verify that the generated snippet > content as well as the link from `Inflater` work fine in the rendered javadoc > HTML. This pull request has now been integrated. Changeset: d8af5894 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/d8af58941b5dedb9774c0971895c4924e57ac28b Stats: 155 lines in 3 files changed: 96 ins; 57 del; 2 mod 8026127: Deflater/Inflater documentation incomplete/misleading Reviewed-by: lancea - PR: https://git.openjdk.org/jdk/pull/19507
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v3]
> Can I please get a review of this doc-only change which proposes to improve > the code snippet that's in `java.util.zip.Deflater` and > `java.util.zip.Inflater` to better explain the usage of those classes? This > addresses https://bugs.openjdk.org/browse/JDK-8026127. > > The commit in the PR cleans up the snippet to correctly compress/decompress > till the `Deflater` and `Inflater` are `finished()`. Additionally, the > snippet also shows that the `Deflater` and `Inflater` are expected to be > closed when their usage it done, to release the resources held by those > instances. > > I've run `make docs-image` locally to verify that the generated snippet > content as well as the link from `Inflater` work fine in the rendered javadoc > HTML. Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision: - minor change to the comment - move the snippet to an external snippet - Changes: - all: https://git.openjdk.org/jdk/pull/19507/files - new: https://git.openjdk.org/jdk/pull/19507/files/098212a6..7a72a760 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19507=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19507=01-02 Stats: 158 lines in 3 files changed: 97 ins; 58 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19507.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507 PR: https://git.openjdk.org/jdk/pull/19507
Re: RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher [v2]
> Can I please get a review for this change which proposes to remove the > `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? > > This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that > JBS issue, in a recent PR discussion, it was suggested > https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this > macro should be removed and the failure of a JNI specified operation (the > ones for which this macro is being used) should be determined based on a > `NULL` value returned from that function. The commit in this PR removes this > macros and updates the call sites to do a `NULL` check. > > Given the nature of this change, no new tests have been added. tier1, tier2 > and tier3 testing passed successfully with these changes. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: improve comments - Changes: - all: https://git.openjdk.org/jdk/pull/19576/files - new: https://git.openjdk.org/jdk/pull/19576/files/1c2a3dfb..1b3d630a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19576=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19576=00-01 Stats: 20 lines in 1 file changed: 0 ins; 8 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19576.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19576/head:pull/19576 PR: https://git.openjdk.org/jdk/pull/19576
RFR: 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher
Can I please get a review for this change which proposes to remove the `CHECK_EXCEPTION_NULL_FAIL` macro from the `java` launcher code? This addresses https://bugs.openjdk.org/browse/JDK-8333714. As noted in that JBS issue, in a recent PR discussion, it was suggested https://github.com/openjdk/jdk/pull/18786#issuecomment-2147452633 that this macro should be removed and the failure of a JNI specified operation (the ones for which this macro is being used) should be determined based on a `NULL` value returned from that function. The commit in this PR removes this macros and updates the call sites to do a `NULL` check. Given the nature of this change, no new tests have been added. tier1, tier2 and tier3 testing passed successfully with these changes. - Commit messages: - simplify function comments - 8333714: Cleanup the usages of CHECK_EXCEPTION_NULL_FAIL macro in java launcher Changes: https://git.openjdk.org/jdk/pull/19576/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19576=00 Issue: https://bugs.openjdk.org/browse/JDK-8333714 Stats: 76 lines in 1 file changed: 45 ins; 9 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19576.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19576/head:pull/19576 PR: https://git.openjdk.org/jdk/pull/19576
Re: RFR: 8026127: Deflater/Inflater documentation incomplete/misleading [v2]
> Can I please get a review of this doc-only change which proposes to improve > the code snippet that's in `java.util.zip.Deflater` and > `java.util.zip.Inflater` to better explain the usage of those classes? This > addresses https://bugs.openjdk.org/browse/JDK-8026127. > > The commit in the PR cleans up the snippet to correctly compress/decompress > till the `Deflater` and `Inflater` are `finished()`. Additionally, the > snippet also shows that the `Deflater` and `Inflater` are expected to be > closed when their usage it done, to release the resources held by those > instances. > > I've run `make docs-image` locally to verify that the generated snippet > content as well as the link from `Inflater` work fine in the rendered javadoc > HTML. Jaikiran Pai 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 two additional commits since the last revision: - merge latest from master branch - 8026127: Deflater/Inflater documentation incomplete/misleading - Changes: - all: https://git.openjdk.org/jdk/pull/19507/files - new: https://git.openjdk.org/jdk/pull/19507/files/d8d86bcb..098212a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19507=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19507=00-01 Stats: 19514 lines in 482 files changed: 12009 ins; 5593 del; 1912 mod Patch: https://git.openjdk.org/jdk/pull/19507.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507 PR: https://git.openjdk.org/jdk/pull/19507
Re: RFR: 8326227: Rounding error that may distort computeNextGaussian results [v3]
On Thu, 9 May 2024 03:55:15 GMT, Chris Hennick wrote: >> Chris Hennick has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Bug fix: add-exports was for wrong package > > Keep open. > > @JimLaskey it looks like you're the author of most of the surrounding code; > can you please confirm that there's a rounding error in the existing > implementation and that this PR will fix it? Or if the `git blame` logs are > giving you more credit than you're willing to take, then could you please > assign it to one ot the real authors or, if that's not possible, to someone > else who's in a relevant role and can capable of reviewing and eventually > merging this PR? Hello Chris @Pr0methean, please keep this open. Some of us are checking if we can find someone experienced with this code to provide reviews. - PR Comment: https://git.openjdk.org/jdk/pull/17703#issuecomment-2151485139
Integrated: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
On Mon, 3 Jun 2024 05:06:00 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. This pull request has now been integrated. Changeset: d7d1afb0 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/d7d1afb0a84e771870e9f43e08c4a63c8fdccdd9 Stats: 21 lines in 2 files changed: 11 ins; 2 del; 8 mod 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE Reviewed-by: lancea, alanb - PR: https://git.openjdk.org/jdk/pull/19515
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]
On Wed, 5 Jun 2024 12:14:07 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which updates the API specification >> of `java.util.zip.InflaterInputStream.skip()` method to match its current >> implementation? >> >> `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes >> a `long` value `n` representing the number of bytes to skip. When a value >> greater than `Integer.MAX_VALUE` is passed to this >> `InflaterInputStream.skip()` method, the current implementation in that >> method only skips at most `Integer.MAX_VALUE` bytes. >> `DeflaterInputStream.skip()` too has the same behaviour. However, in the >> case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that >> method's API documentation. `InflaterInputStream.skip()` currently doesn't >> specify this behaviour. >> >> The commit in this PR updates the `InflaterInputStream.skip()` to specify >> the current implementation. >> >> I'll open a CSR for this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > clarify blocking semantic Thank you all for the reviews and the inputs to the specification text. The CSR has been approved, so I'll integrate this now. - PR Comment: https://git.openjdk.org/jdk/pull/19515#issuecomment-2150413847
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v4]
> Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: clarify blocking semantic - Changes: - all: https://git.openjdk.org/jdk/pull/19515/files - new: https://git.openjdk.org/jdk/pull/19515/files/a43e96de..b7b870d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19515=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=02-03 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v3]
> Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: review feedback updates from the CSR - Changes: - all: https://git.openjdk.org/jdk/pull/19515/files - new: https://git.openjdk.org/jdk/pull/19515/files/e13e65d2..a43e96de Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19515=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=01-02 Stats: 12 lines in 2 files changed: 2 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman @SoniaZaldana CI testing of this PR along with latest master commits passed without any failures. So you can now integrate this. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2148719775
Re: RFR: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3 [v2]
On Tue, 4 Jun 2024 15:17:33 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. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Review feedback: use Reference::refersTo consistently The changes look good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19503#pullrequestreview-2096922118
Re: RFR: 8329581: Java launcher no longer prints a stack trace [v10]
On Fri, 31 May 2024 14:34:16 GMT, Sonia Zaldana Calles wrote: >> Sonia Zaldana Calles has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Decreasing diff size addressing unnecessary changes > > Hi all, > > I think there's some consensus that we need some follow up cleanup issues for > the JNI spec, renaming constants, fixing return codes, etc. > > Seeing how that grows the scope of the issue quite a bit, I'd like to push > this patch and track the other issues brought up separately. > > If there are no objections about the current state, I'd like to integrate > some time next week. Let me know your thoughts. > > cc: @jaikiran, @AlanBateman Hello Sonia @SoniaZaldana, so based on Alan's latest inputs, the next thing to do on this PR is to refresh the PR to merge it with the latest master branch changes and then run tier1, tier2 and tier3 tests to make sure they continue to pass and have this integrated soon. I'll file follow up issue(s) and also trigger CI testing of this PR. - PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2147476871
Re: RFR: 8326951: since-checker - missing @ since tags [v9]
On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla wrote:
>> I added `@since` tags for methods/constructors that do not match the
>> `@since` of the enclosing class.
>>
>> The `write` method already existed in `PrintStream` in earlier versions and
>> instances of it could always call this method, since it extends
>> `FilterOutputStream` [which has the
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>>
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`.
>> And the checker tool differentiates between them because of that.
>>
>> This is similar to #18032 and #18373
>>
>> For context, I am writing tests to check for accurate use of `@since` tags
>> in documentation comments in source code.
>> We're following these rules for now:
>>
>> ### Rule 1: Introduction of New Elements
>>
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must
>> include `@since N`.
>> - Exception: Member elements (fields, methods, nested classes) may omit
>> `@since` if their version matches the value specified for the enclosing
>> class or interface.
>>
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>>
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should
>> not include `@since N`.
>>
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>>
>> - When inspecting methods, prioritize the `@since` annotation of the
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the
>> enclosing element's `@since`.
>>
>> I.e. if A extends B, and we add a method to B in JDK N, and add an
>> override of the method to A in JDK M (M > N), we will use N as the effective
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> empty commit and merge
These changes look good to me. Thank you Nizar for the work on the `@since`
checker and these PRs in individual areas.
src/java.base/share/classes/java/util/zip/Deflater.java line 342:
> 340: * @since 11
> 341: */
> 342: public void setDictionary(ByteBuffer dictionary) {
This method was introduced in Java 11 through
https://bugs.openjdk.org/browse/JDK-6341887. It appears to be an oversight that
a `@since 11` wasn't added to this method in that change, because other new
methods introduced in that change did come with a `@since 11`. So this addition
of `@since 11` now looks fine to me.
-
Marked as reviewed by jpai (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18055#pullrequestreview-2096192264
PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625879138
Re: RFR: 8326951: since-checker - missing @ since tags [v9]
On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla wrote:
>> I added `@since` tags for methods/constructors that do not match the
>> `@since` of the enclosing class.
>>
>> The `write` method already existed in `PrintStream` in earlier versions and
>> instances of it could always call this method, since it extends
>> `FilterOutputStream` [which has the
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>>
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`.
>> And the checker tool differentiates between them because of that.
>>
>> This is similar to #18032 and #18373
>>
>> For context, I am writing tests to check for accurate use of `@since` tags
>> in documentation comments in source code.
>> We're following these rules for now:
>>
>> ### Rule 1: Introduction of New Elements
>>
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must
>> include `@since N`.
>> - Exception: Member elements (fields, methods, nested classes) may omit
>> `@since` if their version matches the value specified for the enclosing
>> class or interface.
>>
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>>
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should
>> not include `@since N`.
>>
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>>
>> - When inspecting methods, prioritize the `@since` annotation of the
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the
>> enclosing element's `@since`.
>>
>> I.e. if A extends B, and we add a method to B in JDK N, and add an
>> override of the method to A in JDK M (M > N), we will use N as the effective
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> empty commit and merge
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
513:
> 511: */
> 512: @Override
> 513: public float nextFloat(float bound) {
These 2 `nextFloat(...)` methods which take parameters were introduced in Java
17 https://bugs.openjdk.org/browse/JDK-8248862, whereas the no-arg
`nextFloat()` method existed prior to that. So adding `@since 17` to these 2
methods looks correct to me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625871382
Re: RFR: 8326951: since-checker - missing @ since tags [v9]
On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla wrote:
>> I added `@since` tags for methods/constructors that do not match the
>> `@since` of the enclosing class.
>>
>> The `write` method already existed in `PrintStream` in earlier versions and
>> instances of it could always call this method, since it extends
>> `FilterOutputStream` [which has the
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>>
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`.
>> And the checker tool differentiates between them because of that.
>>
>> This is similar to #18032 and #18373
>>
>> For context, I am writing tests to check for accurate use of `@since` tags
>> in documentation comments in source code.
>> We're following these rules for now:
>>
>> ### Rule 1: Introduction of New Elements
>>
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must
>> include `@since N`.
>> - Exception: Member elements (fields, methods, nested classes) may omit
>> `@since` if their version matches the value specified for the enclosing
>> class or interface.
>>
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>>
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should
>> not include `@since N`.
>>
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>>
>> - When inspecting methods, prioritize the `@since` annotation of the
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the
>> enclosing element's `@since`.
>>
>> I.e. if A extends B, and we add a method to B in JDK N, and add an
>> override of the method to A in JDK M (M > N), we will use N as the effective
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> empty commit and merge
src/java.base/share/classes/java/util/Properties.java line 190:
> 188: * @since 10
> 189: */
> 190: public Properties(int initialCapacity) {
This constructor was introduced in Java 10 through
https://bugs.openjdk.org/browse/JDK-8189319, so adding `@since 10` here looks
correct to me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625864049
Re: RFR: 8326951: since-checker - missing @ since tags [v9]
On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla wrote: >> I added `@since` tags for methods/constructors that do not match the >> `@since` of the enclosing class. >> >> The `write` method already existed in `PrintStream` in earlier versions and >> instances of it could always call this method, since it extends >> `FilterOutputStream` [which has the >> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96). >> >> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int >> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. >> And the checker tool differentiates between them because of that. >> >> This is similar to #18032 and #18373 >> >> For context, I am writing tests to check for accurate use of `@since` tags >> in documentation comments in source code. >> We're following these rules for now: >> >> ### Rule 1: Introduction of New Elements >> >> - If an element is new in JDK N, with no equivalent in JDK N-1, it must >> include `@since N`. >> - Exception: Member elements (fields, methods, nested classes) may omit >> `@since` if their version matches the value specified for the enclosing >> class or interface. >> >> ### Rule 2: Existing Elements in Subsequent JDK Versions >> >> - If an element exists in JDK N, with an equivalent in JDK N-1, it should >> not include `@since N`. >> >> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since` >> >> - When inspecting methods, prioritize the `@since` annotation of the >> supertype's overridden method. >> - If unavailable or if the enclosing class's `@since` is newer, use the >> enclosing element's `@since`. >> >> I.e. if A extends B, and we add a method to B in JDK N, and add an >> override of the method to A in JDK M (M > N), we will use N as the effective >> `@since` for the method. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > empty commit and merge src/java.base/share/classes/java/nio/MappedByteBuffer.java line 404: > 402: */ > 403: @Override > 404: public abstract MappedByteBuffer slice(); This and the other 3 methods on which this `@since 17` is being added here were introduced in Java 17, through https://bugs.openjdk.org/browse/JDK-4833719. Before that change, these methods would have been available on this `MappedByteBuffer` through the super `ByteBuffer` class and those methods have been specified to return a `ByteBuffer`. After that change in JDK-4833719, these methods now return a `MappedByteBuffer` which is an observable change. So adding `@since 17` to these methods looks correct to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625858949
Re: RFR: 8326951: since-checker - missing @ since tags [v9]
On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla wrote:
>> I added `@since` tags for methods/constructors that do not match the
>> `@since` of the enclosing class.
>>
>> The `write` method already existed in `PrintStream` in earlier versions and
>> instances of it could always call this method, since it extends
>> `FilterOutputStream` [which has the
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>>
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`.
>> And the checker tool differentiates between them because of that.
>>
>> This is similar to #18032 and #18373
>>
>> For context, I am writing tests to check for accurate use of `@since` tags
>> in documentation comments in source code.
>> We're following these rules for now:
>>
>> ### Rule 1: Introduction of New Elements
>>
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must
>> include `@since N`.
>> - Exception: Member elements (fields, methods, nested classes) may omit
>> `@since` if their version matches the value specified for the enclosing
>> class or interface.
>>
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>>
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should
>> not include `@since N`.
>>
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>>
>> - When inspecting methods, prioritize the `@since` annotation of the
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the
>> enclosing element's `@since`.
>>
>> I.e. if A extends B, and we add a method to B in JDK N, and add an
>> override of the method to A in JDK M (M > N), we will use N as the effective
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> empty commit and merge
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7925:
> 7923: * @since 17
> 7924: */
> 7925: public static MethodHandle tableSwitch(MethodHandle fallback,
> MethodHandle... targets) {
This method was introduced in Java 17 through
https://bugs.openjdk.org/browse/JDK-8263087. So this addition of `@since 17`
looks fine to me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625845162
Re: RFR: 8326951: since-checker - missing @ since tags [v9]
On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla wrote:
>> I added `@since` tags for methods/constructors that do not match the
>> `@since` of the enclosing class.
>>
>> The `write` method already existed in `PrintStream` in earlier versions and
>> instances of it could always call this method, since it extends
>> `FilterOutputStream` [which has the
>> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96).
>>
>> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int
>> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`.
>> And the checker tool differentiates between them because of that.
>>
>> This is similar to #18032 and #18373
>>
>> For context, I am writing tests to check for accurate use of `@since` tags
>> in documentation comments in source code.
>> We're following these rules for now:
>>
>> ### Rule 1: Introduction of New Elements
>>
>> - If an element is new in JDK N, with no equivalent in JDK N-1, it must
>> include `@since N`.
>> - Exception: Member elements (fields, methods, nested classes) may omit
>> `@since` if their version matches the value specified for the enclosing
>> class or interface.
>>
>> ### Rule 2: Existing Elements in Subsequent JDK Versions
>>
>> - If an element exists in JDK N, with an equivalent in JDK N-1, it should
>> not include `@since N`.
>>
>> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since`
>>
>> - When inspecting methods, prioritize the `@since` annotation of the
>> supertype's overridden method.
>> - If unavailable or if the enclosing class's `@since` is newer, use the
>> enclosing element's `@since`.
>>
>> I.e. if A extends B, and we add a method to B in JDK N, and add an
>> override of the method to A in JDK M (M > N), we will use N as the effective
>> `@since` for the method.
>
> Nizar Benalla has updated the pull request incrementally with one additional
> commit since the last revision:
>
> empty commit and merge
src/java.base/share/classes/java/lang/reflect/MalformedParameterizedTypeException.java
line 56:
> 54: * @since 10
> 55: */
> 56: public MalformedParameterizedTypeException(String message) {
Both this and the explicit no-arg constructor were introduced in this class
through https://bugs.openjdk.org/browse/JDK-8183175 in Java 10. Since an
(implicit) no-arg constructor was always available even before that change, it
makes sense to add a `@since 10` to only this explicit constructor. This change
looks good.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625848542
Re: RFR: 8326951: since-checker - missing @ since tags [v9]
On Mon, 13 May 2024 20:39:14 GMT, Nizar Benalla wrote: >> I added `@since` tags for methods/constructors that do not match the >> `@since` of the enclosing class. >> >> The `write` method already existed in `PrintStream` in earlier versions and >> instances of it could always call this method, since it extends >> `FilterOutputStream` [which has the >> method](https://github.com/openjdk/jdk6/blob/3e49aa876353eaa215cde71eb21acc9b7f9872a0/jdk/src/share/classes/java/io/FilterOutputStream.java#L96). >> >> for `MappedByteBuffer slice()` and `MappedByteBuffer slice(int index, int >> length)`, the return type changed from `ByteBuffer ` to `MappedByteBuffer`. >> And the checker tool differentiates between them because of that. >> >> This is similar to #18032 and #18373 >> >> For context, I am writing tests to check for accurate use of `@since` tags >> in documentation comments in source code. >> We're following these rules for now: >> >> ### Rule 1: Introduction of New Elements >> >> - If an element is new in JDK N, with no equivalent in JDK N-1, it must >> include `@since N`. >> - Exception: Member elements (fields, methods, nested classes) may omit >> `@since` if their version matches the value specified for the enclosing >> class or interface. >> >> ### Rule 2: Existing Elements in Subsequent JDK Versions >> >> - If an element exists in JDK N, with an equivalent in JDK N-1, it should >> not include `@since N`. >> >> ### Rule 3: Handling Missing `@since` Tags in methods if there is no `@since` >> >> - When inspecting methods, prioritize the `@since` annotation of the >> supertype's overridden method. >> - If unavailable or if the enclosing class's `@since` is newer, use the >> enclosing element's `@since`. >> >> I.e. if A extends B, and we add a method to B in JDK N, and add an >> override of the method to A in JDK M (M > N), we will use N as the effective >> `@since` for the method. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > empty commit and merge src/java.base/share/classes/java/io/PrintStream.java line 683: > 681: * @see #write(byte[],int,int) > 682: * > 683: * @since 14 The `@since 14` was added here as part of https://bugs.openjdk.org/browse/JDK-8187898. The CSR explains what the changes were https://bugs.openjdk.org/browse/JDK-8230625. As noted in that CSR's specification (and attached images), the change for this method in that issue ended up being just API clarification (through a `@apiNote`) and no other change to this specific method. It continued to have the same signature including the throws clause that was previously available on this class through the super `FilterOutputStream#write(byte[])` method. So removing this `@since 14` looks correct to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/18055#discussion_r1625840895
Integrated: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java
On Mon, 3 Jun 2024 04:25:38 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which uncomments an > additional test that was commented out in > `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`? > > As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue > due to which this line was commented out in the test, has been fixed several > releases back and thus this line can now be uncommented. > > I've verified that this test continues to pass with these changes. This pull request has now been integrated. Changeset: d230b303 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/d230b30353f59135287436b09949b80e9fd73a93 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod 898: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java Reviewed-by: iris, lancea - PR: https://git.openjdk.org/jdk/pull/19514
Re: RFR: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java
On Mon, 3 Jun 2024 04:25:38 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which uncomments an > additional test that was commented out in > `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`? > > As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue > due to which this line was commented out in the test, has been fixed several > releases back and thus this line can now be uncommented. > > I've verified that this test continues to pass with these changes. Thank you Iris and Lance for the reviews. > We should look at at a later date to convert this to junit and when we do, > look to rework isMultiReleaseJar() as it actually runs multiple tests within > the test itself and if one fails, the rest will not be run I have added it to my TODO to update this test in future. - PR Comment: https://git.openjdk.org/jdk/pull/19514#issuecomment-2146416470
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
On Mon, 3 Jun 2024 05:06:00 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. The CSR is available for review at https://bugs.openjdk.org/browse/JDK-8333400 - PR Comment: https://git.openjdk.org/jdk/pull/19515#issuecomment-2144323834
Re: RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE [v2]
> Can I please get a review of this change which updates the API specification > of `java.util.zip.InflaterInputStream.skip()` method to match its current > implementation? > > `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a > `long` value `n` representing the number of bytes to skip. When a value > greater than `Integer.MAX_VALUE` is passed to this > `InflaterInputStream.skip()` method, the current implementation in that > method only skips at most `Integer.MAX_VALUE` bytes. > `DeflaterInputStream.skip()` too has the same behaviour. However, in the case > of `DeflaterInputStream.skip()`, this semantic is clearly noted in that > method's API documentation. `InflaterInputStream.skip()` currently doesn't > specify this behaviour. > > The commit in this PR updates the `InflaterInputStream.skip()` to specify the > current implementation. > > I'll open a CSR for this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: also note that the method may block, just like DeflaterInputStream specifies it - Changes: - all: https://git.openjdk.org/jdk/pull/19515/files - new: https://git.openjdk.org/jdk/pull/19515/files/2a116fe4..e13e65d2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19515=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19515=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
RFR: 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE
Can I please get a review of this change which updates the API specification of `java.util.zip.InflaterInputStream.skip()` method to match its current implementation? `InflaterInputStream.skip()`, just like `DeflaterInputStream.skip()`, takes a `long` value `n` representing the number of bytes to skip. When a value greater than `Integer.MAX_VALUE` is passed to this `InflaterInputStream.skip()` method, the current implementation in that method only skips at most `Integer.MAX_VALUE` bytes. `DeflaterInputStream.skip()` too has the same behaviour. However, in the case of `DeflaterInputStream.skip()`, this semantic is clearly noted in that method's API documentation. `InflaterInputStream.skip()` currently doesn't specify this behaviour. The commit in this PR updates the `InflaterInputStream.skip()` to specify the current implementation. I'll open a CSR for this change. - Commit messages: - 8206447: InflaterInputStream.skip receives long but it's limited to Integer.MAX_VALUE Changes: https://git.openjdk.org/jdk/pull/19515/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19515=00 Issue: https://bugs.openjdk.org/browse/JDK-8206447 Stats: 10 lines in 2 files changed: 7 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19515.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19515/head:pull/19515 PR: https://git.openjdk.org/jdk/pull/19515
RFR: 8333398: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java
Can I please get a review of this test-only change which uncomments an additional test that was commented out in `test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java`? As noted in https://bugs.openjdk.org/browse/JDK-898, the original issue due to which this line was commented out in the test, has been fixed several releases back and thus this line can now be uncommented. I've verified that this test continues to pass with these changes. - Commit messages: - 898: Uncomment the commented test in test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarAPI.java Changes: https://git.openjdk.org/jdk/pull/19514/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19514=00 Issue: https://bugs.openjdk.org/browse/JDK-898 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19514.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19514/head:pull/19514 PR: https://git.openjdk.org/jdk/pull/19514
Integrated: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails
On Fri, 31 May 2024 00:57:18 GMT, Jaikiran Pai wrote: > Can I please get a review of this test-only change which updates a couple of > places in the test to use `try-with-resource`? > > As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should > prevent leaking of resources in case there's any failure in the test. The > test continues to pass with this change. This pull request has now been integrated. Changeset: 4785461f Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/4785461f61d8f5c7444d2e6fd90f1e083dbc6fe4 Stats: 128 lines in 1 file changed: 34 ins; 68 del; 26 mod 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails Reviewed-by: lancea - PR: https://git.openjdk.org/jdk/pull/19492
Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails [v2]
On Sat, 1 Jun 2024 05:18:17 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which updates a couple of >> places in the test to use `try-with-resource`? >> >> As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should >> prevent leaking of resources in case there's any failure in the test. The >> test continues to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > convert the test to junit Thank you Lance for the review. - PR Comment: https://git.openjdk.org/jdk/pull/19492#issuecomment-2143653405
Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails [v2]
On Sat, 1 Jun 2024 05:18:17 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test-only change which updates a couple of >> places in the test to use `try-with-resource`? >> >> As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should >> prevent leaking of resources in case there's any failure in the test. The >> test continues to pass with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > convert the test to junit Hello Lance, I have now updated the PR to convert this test to junit test. While at it, I have also cleaned up the code to use `java.nio.file.Path` and use the jtreg scratch directory for the directories and the jar file this test creates. That way, we don't have to manually delete the directories when done. I have verified that the changes don't change the semantics of what was being tested in this test, which was a regression test for https://bugs.openjdk.org/browse/JDK-6374379. The test continues to pass with these change in our CI against all platforms. - PR Comment: https://git.openjdk.org/jdk/pull/19492#issuecomment-2143307371
Re: RFR: 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.
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?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19503#discussion_r1623151595
Re: RFR: 7022325: TEST_BUG: test/java/util/zip/ZipFile/ReadLongZipFileName.java leaks files if it fails [v2]
> Can I please get a review of this test-only change which updates a couple of > places in the test to use `try-with-resource`? > > As noted in https://bugs.openjdk.org/browse/JDK-7022325 this change should > prevent leaking of resources in case there's any failure in the test. The > test continues to pass with this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: convert the test to junit - Changes: - all: https://git.openjdk.org/jdk/pull/19492/files - new: https://git.openjdk.org/jdk/pull/19492/files/3ce9ca81..b9d56006 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19492=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19492=00-01 Stats: 122 lines in 1 file changed: 28 ins; 60 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/19492.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19492/head:pull/19492 PR: https://git.openjdk.org/jdk/pull/19492
RFR: 8026127: Deflater/Inflater documentation incomplete/misleading
Can I please get a review of this doc-only change which proposes to improve the code snippet that's in `java.util.zip.Deflater` and `java.util.zip.Inflater` to better explain the usage of those classes? This addresses https://bugs.openjdk.org/browse/JDK-8026127. The commit in the PR cleans up the snippet to correctly compress/decompress till the `Deflater` and `Inflater` are `finished()`. Additionally, the snippet also shows that the `Deflater` and `Inflater` are expected to be closed when their usage it done, to release the resources held by those instances. I've run `make docs-image` locally to verify that the generated snippet content as well as the link from `Inflater` work fine in the rendered javadoc HTML. - Commit messages: - 8026127: Deflater/Inflater documentation incomplete/misleading Changes: https://git.openjdk.org/jdk/pull/19507/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19507=00 Issue: https://bugs.openjdk.org/browse/JDK-8026127 Stats: 88 lines in 2 files changed: 31 ins; 31 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/19507.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19507/head:pull/19507 PR: https://git.openjdk.org/jdk/pull/19507
Integrated: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater
On Thu, 30 May 2024 12:14:53 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to address the issue > noted in https://bugs.openjdk.org/browse/JDK-8210471? > > `java.util.zip.Inflater` when instantiated will hold on the native resources > which are freed only when `Inflater.end()` is called. The constructor of > `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal > use. After instantiating the `Inflater`, the `GZIPInputStream` constructor > before returning from the constructor, can run into either a > `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` > when trying to read a GZIP header from the underlying `InputStream`. In > either of these cases, the `Inflater` that the `GZIPInputStream` created > internally will end up leaking and the caller has no way to `end()` that > `Inflater` or even knowledge of that `Inflater`. > > The commit in this PR catches the `IOException` when reading the GZIP header > and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an > `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` > will now check the `InputStream` to be non-null and `size` to be `>0`, both > of which were being done by the `super` constructor. > > Given the nature of the change, no new test has been added. Existing tests in > `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 testing > is in progress. This pull request has now been integrated. Changeset: d9e7b7e7 Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/d9e7b7e7da98a0170d26301a4bbd61aad0127c6e Stats: 118 lines in 2 files changed: 116 ins; 0 del; 2 mod 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater Reviewed-by: lancea - PR: https://git.openjdk.org/jdk/pull/19475
Re: RFR: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater [v4]
On Fri, 31 May 2024 11:03:18 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to address the issue >> noted in https://bugs.openjdk.org/browse/JDK-8210471? >> >> `java.util.zip.Inflater` when instantiated will hold on the native resources >> which are freed only when `Inflater.end()` is called. The constructor of >> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal >> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor >> before returning from the constructor, can run into either a >> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` >> when trying to read a GZIP header from the underlying `InputStream`. In >> either of these cases, the `Inflater` that the `GZIPInputStream` created >> internally will end up leaking and the caller has no way to `end()` that >> `Inflater` or even knowledge of that `Inflater`. >> >> The commit in this PR catches the `IOException` when reading the GZIP header >> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an >> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` >> will now check the `InputStream` to be non-null and `size` to be `>0`, both >> of which were being done by the `super` constructor. >> >> Given the nature of the change, no new test has been added. Existing tests >> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 >> testing is in progress. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > rework the test to make it parameterized Thank you Lance for the review. tier testing went fine with the latest state in this PR. I'll go ahead and integrate this now. - PR Comment: https://git.openjdk.org/jdk/pull/19475#issuecomment-2142193675
