Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v5]

[Stefan Karlsson]

> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename 
> createJavaProcessBuilder' changed the name of the ProcessTools helper 
> functions used to create `ProcessBuilder`s used to spawn new java test 
> processes.
> 
> We now have `createTestJavaProcessBuilder` and 
> `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, 
> while the latter doesn't.
> 
> With these functions it is common to see the following pattern in tests:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = executeProcess(pb);
> 
> 
> We have a couple of thin wrapper in `ProcessTools` that does exactly this, so 
> that the code can be written as a one-liner:
> 
> OutputAnalyzer output = ProcessTools.executeTestJvm();
> 
> 
> I propose that we name this functions using the same naming scheme we used 
> for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. 
> That is, we change `executeTestJvm` to `executeTestJava` and add a new 
> `executeLimitedTestJava` function.

Stefan Karlsson has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains five additional 
commits since the last revision:

 - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm
 - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm
 - Test cleanup
 - Fix impl and add test
 - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17049/files
  - new: https://git.openjdk.org/jdk/pull/17049/files/486dc6d5..755d925d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17049=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=17049=03-04

  Stats: 875 lines in 70 files changed: 577 ins; 58 del; 240 mod
  Patch: https://git.openjdk.org/jdk/pull/17049.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049

PR: https://git.openjdk.org/jdk/pull/17049

Integrated: 8321718: ProcessTools.executeProcess calls waitFor before logging

[Stefan Karlsson]

On Mon, 11 Dec 2023 11:16:05 GMT, Stefan Karlsson  wrote:

> There is some logging printed when tests spawns processes. This logging is 
> triggered from calls to `OutputAnalyzer`, when it delegates calls to 
> `LazyOutputBuffer`.
> 
> If we write code like this:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = new OutputAnalyzer(pb.start());
> int exitValue = output.getExitValue();
> 
> 
> We get the following logging:
> 
> [timestamp0] "Gathering output for process 
> [timestamp1] Waiting for completion for process 
> [timestamp2] Waiting for completion finished for process 
> 
> 
> The first line comes from the `OutputAnalyzer` constructor and the two other 
> lines comes from the `getExitValue` call, which calls logs the above messages 
> around the call to `waitFor`.
> 
> If instead write the code above as:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = ProcessTools.executeProcess(pb);
> int exitValue = output.getExitValue();
> 
> 
> We get the same logging, but timestamp1 and timestamp2 are almost the same. 
> This happens because there's an extra call to `waitFor` inside 
> `executeProcess`, but that `waitFor` does not have the "wait for" logging. 
> So, instead we get the logging for the no-op `waitFor` inside `getExitValue`.
> 
> I would like to propose a small workaround to solve this. The workaround is 
> that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` 
> via `OutputAnalyzer`.  This is a small, limited workaround for this issue. 
> Ideally I would like to extract the entire Process handling code out of 
> LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all 
> usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the 
> test directories, so I'm starting out with this suggestion.
> 
> We can see of this patch by looking at the timestamps generated from the 
> included test. Without the proposed workaround:
> 
> Without
> 
> testExecuteProcessExit
> [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719
> [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719
> [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process 
> 2547719
> 
> testExecuteProcessStdout
> [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741
> [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741
> [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process 
> 2547741
> 
> 
> testNewOutputAnalyzerExit
> [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782
> [2023...

This pull request has now been integrated.

Changeset: 9ab29f8d
Author:Stefan Karlsson 
URL:   
https://git.openjdk.org/jdk/commit/9ab29f8dcd1c0092e4251f996bd53c704e87a74a
Stats: 61 lines in 3 files changed: 47 ins; 11 del; 3 mod

8321718: ProcessTools.executeProcess calls waitFor before logging

Reviewed-by: dholmes, jpai

-

PR: https://git.openjdk.org/jdk/pull/17052

RFR: JDK-8322878: Including sealing information Class.toGenericString()

[Joe Darcy]

As recently discussed on core libs, sealed-ness information could be included 
in the Class.toGenericString() output, analagous to how "modifiers" that also 
correspond to JVM access flags are handled.

This is the initial spec, implementation, and test updated needed for that 
change. If there is consensus this is a reasonable direction, I'll create the 
CSR, etc.

-

Commit messages:
 - JDK-8322878: Including sealing information Class.toGenericString()

Changes: https://git.openjdk.org/jdk/pull/17239/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17239=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322878
  Stats: 25 lines in 2 files changed: 16 ins; 1 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/17239.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17239/head:pull/17239

PR: https://git.openjdk.org/jdk/pull/17239

Re: Blessed modifier order does not include sealed/non-sealed

[Joseph D. Darcy]

Note that in terms of updating the implementation of toGenericString() 
JDK-8322878 to included sealed information, since strictfp is a no-op in 
the same release sealed/non-sealed was added, a class file can be


    sealed XOR strictfp

Therefore, Class.toGenericString() could print the modifiers from 
getModifiers() and then add sealing information while retaining the 
blessed modifier order. In other words, the 
"Modifier.toString(modifiers)" code in Class.toGenericString() does not 
have to be "interrupted" to handle presenting sealed information in the 
blessed order.


HTH,

-Joe

On 1/2/2024 4:08 AM, Pavel Rappo wrote:

I assume the order for `sealed` and `non-sealed` has effectively been decided 
by JLS: https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1

 8.1.1. Class Modifiers
 ...
 
 ClassModifier:

 (one of)
 Annotation public protected private
 abstract static final sealed non-sealed strictfp
 
 ...


 If two or more (distinct) class modifiers appear in a class declaration, 
then it is customary, though not required, that they appear in the order 
consistent with that shown above in the production for ClassModifier.


Shall I just create a PR?


On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:

I couldn't find any prior discussions on this matter.

I noticed that bin/blessed-modifier-order.sh has not been updated for the [recently 
introduced](https://openjdk.org/jeps/409) `sealed` and `non-sealed` keywords. I also note 
that we already have cases in OpenJDK where those keywords are ordered differently. If we 
have a consensus on how to extend the "blessed order" onto those new keywords, 
I can create a PR to update the script.

-Pavel

Re: Blessed modifier order does not include sealed/non-sealed

[Joseph D. Darcy]

Hello,

Some time back, the java.lang.reflect package level docs had a brief 
discussion added concerning what exactly core reflection is modeling. 
[1]. tl;dr -- mostly a class file level view. As such, a contingent 
property of the design of JEP 409 is that sealed/non-sealed happens to 
_not_ to expressed JVM-level access flags/modifiers. However, it would 
be reasonable IMO for Class.toGenericString(), but not Class.toString(), 
to be updated to expose some sealing-related information. I've filed 
JDK-8322878: "Including sealing information Class.toGenericString()."


-Joe

[1] Added under     JDK-8262807: "Note assumptions of core reflection 
modeling and parameter handling" in JDK 17:



Java programming language and JVM modeling in core reflection

The components of core reflection, which include types in this package 
as well as Class, Package, and Module, fundamentally present a JVM 
model of the entities in question rather than a Java programming 
language model. A Java compiler, such as javac, translates Java source 
code into executable output that can be run on a JVM, primarily class 
files. Compilers for source languages other than Java can and do 
target the JVM as well.


The translation process, including from Java language sources, to 
executable output for the JVM is not a one-to-one mapping. Structures 
present in the source language may have no representation in the 
output and structures not present in the source language may be 
present in the output. The latter are called synthetic structures. 
Synthetic structures can include methods, fields, parameters, classes 
and interfaces. One particular kind of synthetic method is a bridge 
method. It is possible a synthetic structure may not be marked as 
such. In particular, not all class file versions support marking a 
parameter as synthetic. A source language compiler generally has 
multiple ways to translate a source program into a class file 
representation. The translation may also depend on the version of the 
class file format being targeted as different class file versions have 
different capabilities and features. In some cases the modifiers 
present in the class file representation may differ from the modifiers 
on the originating element in the source language, including final on 
a parameter and protected, private, and static on classes and interfaces.


Besides differences in structural representation between the source 
language and the JVM representation, core reflection also exposes 
runtime specific information. For example, the class loaders and 
protection domains of a Class are runtime concepts without a direct 
analogue in source code.


https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/reflect/package-summary.html

On 1/2/2024 8:56 AM, Roger Riggs wrote:

Hi Pavel,

It better to look to javax.lang.model.element.Modifier 
 
for the language view of the class.


java.lang.reflect.Modifier covers the modifier flags as represented in 
the class file and defined in the JVMS.
* The values for the constants * representing the modifiers are taken 
from the tables in sections * {@jvms 4.1}, {@jvms 4.4}, {@jvms 4.5}, 
and {@jvms 4.7} of * The Java Virtual Machine Specification.
Sealing is represented in the class file as a non-empty list of 
permitted classes. Hence the method of java.lang.Class.


Since java.lang.Modifier.toString is based on the flag bits from the 
class file, "sealed" would not appear in any string it generates.



It might be possible to inject a comment in the toString method 
similar to the comment about interface not being a true modifier and 
including a reference to the javax.lang.model.element.Modifier enum.


Roger


On 1/2/24 11:31 AM, Pavel Rappo wrote:

Hi Roger,

Happy New Year to you too!

Although it's a _somewhat_ separate issue, I found that the shell script refers 
to java.lang.reflect.Modifier#toString which does NOT mention either `sealed` 
or `non-sealed`. More precisely, the script refers to the JDK 8 version of that 
method, but [the 
method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int))
  hasn't changed since 2009 and states that:

 ...The modifier names are returned in an order consistent with the 
suggested modifier orderings given in sections 8.1.1, 8.3.1, 8.4.3, 8.8.3, and 
9.1.1 of The Java Language Specification. The full modifier ordering used by 
this method is:

 public protected private abstract static final transient volatile 
synchronized native strictfp interface

It does not seem like `sealed` and `non-sealed` are even modelled by 
java.lang.reflect.Modifier, although `sealed` is modelled by 
`java.lang.Class#isSealed`. It cannot be overlook, can it?


On 2 Jan 2024, at 14:38, Roger Riggs  wrote:

Hi Pavel,

yes, a PR would be next.

Happy New Year, Roger

On 1/2/24 

Re: RFR: 8322322: Support archived full module graph when -Xbootclasspath/a is used [v3]

[Calvin Cheung]

> Please review this change for enabling full module graph even when 
> -Xbootclasspath/a is specified. The validation of -Xbootclasspath/a is 
> already being done in `FileMapInfo::validate_boot_class_paths()`. The full 
> module graph will be disabled if there's a mismatch in -Xbootclasspath/a 
> between dump time and runtime.
> 
> The changes in ClassLoaders.java is for setting up the append class path for 
> the boot loader retrieved from the CDS archive. It is required because some 
> existing tests are using the `getResourceAsStream()` api. Those tests would 
> fail without the change.
> 
> Passed tiers 1 - 4 testing.

Calvin Cheung has updated the pull request incrementally with one additional 
commit since the last revision:

  tests update per discussion with Ioi

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17178/files
  - new: https://git.openjdk.org/jdk/pull/17178/files/d06b98ff..a2e9f668

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17178=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=17178=01-02

  Stats: 4 lines in 2 files changed: 1 ins; 1 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/17178.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17178/head:pull/17178

PR: https://git.openjdk.org/jdk/pull/17178

Re: Blessed modifier order does not include sealed/non-sealed

[Pavel Rappo]

While we are here, I also note that the script does not explicitly take into 
account the `default` modifier on an interface method. However, after reading 
https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html#jls-9.4 it seems 
that whenever `default` appears, it should be alone one way or another; thus, 
ordering is not an issue.

Indeed, here are the modifier possibilities for an interface method declaration:

public private abstract default static strictfp

And here are the relevant constraints:

  - explicit public is discouraged
  - abstract, default, and static are mutually exclusive
  - strictfp is obsolete
  - private and default are mutually exclusive

So if `default` appears, except for uncompilable, discouraged, or obsolete 
cases, it should be alone. Thoughts?

> On 2 Jan 2024, at 17:23, Roger Riggs  wrote:
> 
> Hi Pavel,
> 
> I agree, the script should refer to the JLS.
> Some people look to the javadoc but its not authoritative on the language.
> 
> Roger
> 
> On 1/2/24 12:18 PM, Pavel Rappo wrote:
>> Right, the language model (javax.lang.model) is a better target to refer to 
>> when considering _source_ files. However, java.lang.reflect.Modifier 
>> corresponds to a set returned by 
>> javax.lang.model.element.Element#getModifiers, not to a single instance of 
>> javax.lang.model.element.Modifier; the order of that set is unspecified.
>> 
>> So perhaps the right thing would be to directly refer to JLS from the 
>> script. What do you think?
>> 
>>> On 2 Jan 2024, at 16:56, Roger Riggs  wrote:
>>> 
>>> Hi Pavel,
>>> 
>>> It better to look to javax.lang.model.element.Modifier for the language 
>>> view of the class.
>>> 
>>> java.lang.reflect.Modifier covers the modifier flags as represented in the 
>>> class file and defined in the JVMS.
>>> * The values for the constants
>>> * representing the modifiers are taken from the tables in sections
>>> * {@jvms 4.1}, {@jvms 4.4}, {@jvms 4.5}, and {@jvms 4.7} of
>>> * The Java Virtual Machine Specification.
>>> Sealing is represented in the class file as a non-empty list of permitted 
>>> classes. Hence the method of java.lang.Class.
>>> 
>>> Since java.lang.Modifier.toString is based on the flag bits from the class 
>>> file, "sealed" would not appear in any string it generates.
>>> 
>>> 
>>> It might be possible to inject a comment in the toString method similar to 
>>> the comment about interface not being a true modifier and including a 
>>> reference to the javax.lang.model.element.Modifier enum.
>>> 
>>> Roger
>>> 
>>> 
>>> On 1/2/24 11:31 AM, Pavel Rappo wrote:
 Hi Roger,
 
 Happy New Year to you too!
 
 Although it's a _somewhat_ separate issue, I found that the shell script 
 refers to java.lang.reflect.Modifier#toString which does NOT mention 
 either `sealed` or `non-sealed`. More precisely, the script refers to the 
 JDK 8 version of that method, but [the 
 method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int))
  hasn't changed since 2009 and states that:
 
 ...The modifier names are returned in an order consistent with the 
 suggested modifier orderings given in sections 8.1.1, 8.3.1, 8.4.3, 8.8.3, 
 and 9.1.1 of The Java Language Specification. The full modifier ordering 
 used by this method is:
 
 public protected private abstract static final transient volatile 
 synchronized native strictfp interface
 
 It does not seem like `sealed` and `non-sealed` are even modelled by 
 java.lang.reflect.Modifier, although `sealed` is modelled by 
 `java.lang.Class#isSealed`. It cannot be overlook, can it?
 
 
> On 2 Jan 2024, at 14:38, Roger Riggs  wrote:
> 
> Hi Pavel,
> 
> yes, a PR would be next.
> 
> Happy New Year, Roger
> 
> On 1/2/24 7:08 AM, Pavel Rappo wrote:
> 
>> I assume the order for `sealed` and `non-sealed` has effectively been 
>> decided by JLS: 
>> https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1
>> 
>> 8.1.1. Class Modifiers
>> ...
>> ClassModifier:
>> (one of)
>> Annotation public protected private
>> abstract static final sealed non-sealed strictfp
>> ...
>> 
>> If two or more (distinct) class modifiers appear in a class declaration, 
>> then it is customary, though not required, that they appear in the order 
>> consistent with that shown above in the production for ClassModifier.
>> 
>> 
>> Shall I just create a PR?
>> 
>> 
>>> On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:
>>> 
>>> I couldn't find any prior discussions on this matter.
>>> 
>>> I noticed that bin/blessed-modifier-order.sh has not been updated for 
>>> the [recently introduced](https://openjdk.org/jeps/409) `sealed` and 
>>> `non-sealed` keywords. I also note that we already have cases in 
>>> OpenJDK where those 

Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]

[Eirik Bjørsnøs]

On Tue, 2 Jan 2024 12:21:21 GMT, Eirik Bjørsnøs  wrote:

>> Please consider this PR which makes `DeflaterOutputStream.close()` always 
>> close its wrapped output stream exactly once.
>> 
>> Currently, closing of the wrapped output stream happens outside the finally 
>> block where `finish()` is called. If `finish()` throws, this means the 
>> wrapped stream will not be closed. This can potentially lead to leaking 
>> resources such as file descriptors or sockets.
>> 
>> This fix is to move the closing of the wrapped stream inside the finally 
>> block.
>> 
>> Additionally, the `closed = true;` statement is moved to the start of the 
>> close method. This makes sure we only ever close the wrapped stream once 
>> (this aligns with the overridden method `FilterOutputStream.close´)
>> 
>> Specification: This change brings the implementation of 
>> `DeflaterOutputStream.close()` in line with its specification:  *Writes 
>> remaining compressed data to the output stream and closes the underlying 
>> stream.*
>> 
>> Risk: This is a behavioural change. There is a small risk that existing code 
>> depends on the close method not following its specification.
>> 
>> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which 
>> simulates the failure condition and verifies that the wrapped stream was 
>> closed under failing and non-failing conditions.
>
> Eirik Bjørsnøs has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Add more extensive testing of combined write/close failure modes
>  - Don't suppress if finishException is null, mark stream as closed even when 
> closing the wrapped stream failed

A CSR for this change has been proposed and is ready for review: 
[JDK-8322871](https://bugs.openjdk.org/browse/JDK-8322871)

-

PR Comment: https://git.openjdk.org/jdk/pull/17209#issuecomment-1874623292

Integrated: 8322868: java/io/BufferedInputStream/TransferToTrusted.java has bad copyright header

[Brian Burkhalter]

On Tue, 2 Jan 2024 20:27:29 GMT, Brian Burkhalter  wrote:

> Add missing comma after 2024.

This pull request has now been integrated.

Changeset: c2477a5c
Author:Brian Burkhalter 
URL:   
https://git.openjdk.org/jdk/commit/c2477a5cad6539e6e38cc0732383aaa2a8df801f
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8322868: java/io/BufferedInputStream/TransferToTrusted.java has bad copyright 
header

Reviewed-by: dcubed

-

PR: https://git.openjdk.org/jdk/pull/17228

Re: RFR: 8322868: java/io/BufferedInputStream/TransferToTrusted.java has bad copyright header

[Daniel D . Daugherty]

On Tue, 2 Jan 2024 20:27:29 GMT, Brian Burkhalter  wrote:

> Add missing comma after 2024.

Thumbs up. This is a trivial fix.

-

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17228#pullrequestreview-1800974980

Integrated: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag

[Eirik Bjørsnøs]

On Sun, 31 Dec 2023 18:07:33 GMT, Eirik Bjørsnøs  wrote:

> Please review this test-only PR which adds test coverage for 
> `ZipFile.getEntry` under certain charset conditions. 
> 
> When `ZipFile.getEntry` is called for an entry which has the `Language 
> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
> unconditionally, even when a different charset was supplied to the `ZipFile` 
> constructor.
> 
> It turns out we do not have any testing for this particular case, as can be 
> verified by commenting out the following line of code in 
> `ZipFile.Source.getEntryPos`:
> 
> 
> //ZipCoder zc = zipCoderForPos(pos);
> ``` 
> 
> and then running `make test TEST="test/jdk/java/util/zip"`
> 
> The current test verifies that the correct ZipCoder is used by 
> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
> 
> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
> (accidentally?) fixed by 
> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
> worthwhile to add explicit testing for this case to avoid regressions.
> 
> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
> JUnit 5. The conversion and modernization of the code is done in the first 
> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
> required to verify the `Language encoding flag` condition for 
> `ZipFile.getEntry`.
> 
> Testing: Verified that the test indeed fails when 
> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
> suggested above.

This pull request has now been integrated.

Changeset: 2cf5f013
Author:Eirik Bjørsnøs 
URL:   
https://git.openjdk.org/jdk/commit/2cf5f0139740c6d85848fc1691e144a6ed1a
Stats: 168 lines in 1 file changed: 96 ins; 22 del; 50 mod

8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' 
flag

Reviewed-by: lancea, jpai

-

PR: https://git.openjdk.org/jdk/pull/17207

RFR: 8322868: java/io/BufferedInputStream/TransferToTrusted.java has bad copyright header

[Brian Burkhalter]

Add missing comma after 2024.

-

Commit messages:
 - 8322868: java/io/BufferedInputStream/TransferToTrusted.java has bad 
copyright header

Changes: https://git.openjdk.org/jdk/pull/17228/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17228=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322868
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17228.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17228/head:pull/17228

PR: https://git.openjdk.org/jdk/pull/17228

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v6]

[Lance Andersen]

On Tue, 2 Jan 2024 17:50:51 GMT, Eirik Bjørsnøs  wrote:

>> Please review this test-only PR which adds test coverage for 
>> `ZipFile.getEntry` under certain charset conditions. 
>> 
>> When `ZipFile.getEntry` is called for an entry which has the `Language 
>> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
>> unconditionally, even when a different charset was supplied to the `ZipFile` 
>> constructor.
>> 
>> It turns out we do not have any testing for this particular case, as can be 
>> verified by commenting out the following line of code in 
>> `ZipFile.Source.getEntryPos`:
>> 
>> 
>> //ZipCoder zc = zipCoderForPos(pos);
>> ``` 
>> 
>> and then running `make test TEST="test/jdk/java/util/zip"`
>> 
>> The current test verifies that the correct ZipCoder is used by 
>> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
>> 
>> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
>> (accidentally?) fixed by 
>> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
>> worthwhile to add explicit testing for this case to avoid regressions.
>> 
>> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
>> JUnit 5. The conversion and modernization of the code is done in the first 
>> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
>> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
>> required to verify the `Language encoding flag` condition for 
>> `ZipFile.getEntry`.
>> 
>> Testing: Verified that the test indeed fails when 
>> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
>> suggested above.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fine tuning assertion messages for consistency between tests

Marked as reviewed by lancea (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1800965421

Integrated: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted

[Sergey Tsypanov]

On Wed, 29 Nov 2023 11:57:37 GMT, Sergey Tsypanov  wrote:

> It looks like we can skip copying of `byte[]` in 
> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
> `java.io`.
> 
> See comment by @vlsi in 
> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612

This pull request has now been integrated.

Changeset: 38042ad4
Author:Sergey Tsypanov 
Committer: Brian Burkhalter 
URL:   
https://git.openjdk.org/jdk/commit/38042ad4e9b57d79cd795fd22d31be63924e34c5
Stats: 114 lines in 2 files changed: 111 ins; 0 del; 3 mod

8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is 
trusted

Reviewed-by: alanb, bpb

-

PR: https://git.openjdk.org/jdk/pull/16879

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v20]

[Sergey Tsypanov]

> It looks like we can skip copying of `byte[]` in 
> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
> `java.io`.
> 
> See comment by @vlsi in 
> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612

Sergey Tsypanov has updated the pull request incrementally with one additional 
commit since the last revision:

  Update TransferToTrusted.java

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16879/files
  - new: https://git.openjdk.org/jdk/pull/16879/files/e769dfb2..8d15e748

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=16879=19
 - incr: https://webrevs.openjdk.org/?repo=jdk=16879=18-19

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/16879.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16879/head:pull/16879

PR: https://git.openjdk.org/jdk/pull/16879

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v20]

[Brian Burkhalter]

On Tue, 2 Jan 2024 20:00:36 GMT, Sergey Tsypanov  wrote:

>> It looks like we can skip copying of `byte[]` in 
>> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
>> `java.io`.
>> 
>> See comment by @vlsi in 
>> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612
>
> Sergey Tsypanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update TransferToTrusted.java

Marked as reviewed by bpb (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/16879#pullrequestreview-1800941486

Re: Blessed modifier order does not include sealed/non-sealed

[Remi Forax]

- Original Message -
> From: "Pavel Rappo" 
> To: "Roger Riggs" 
> Cc: "core-libs-dev" 
> Sent: Tuesday, January 2, 2024 5:31:06 PM
> Subject: Re: Blessed modifier order does not include sealed/non-sealed

> Hi Roger,
> 
> Happy New Year to you too!
> 
> Although it's a _somewhat_ separate issue, I found that the shell script 
> refers
> to java.lang.reflect.Modifier#toString which does NOT mention either `sealed`
> or `non-sealed`. More precisely, the script refers to the JDK 8 version of 
> that
> method, but [the
> method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int))
> hasn't changed since 2009 and states that:
> 
>...The modifier names are returned in an order consistent with the 
> suggested
>modifier orderings given in sections 8.1.1, 8.3.1, 8.4.3, 8.8.3, and 9.1.1 
> of
>The Java Language Specification. The full modifier ordering used by this 
> method
>is:
> 
>public protected private abstract static final transient volatile 
> synchronized
>native strictfp interface
> 
> It does not seem like `sealed` and `non-sealed` are even modelled by
> java.lang.reflect.Modifier, although `sealed` is modelled by
> `java.lang.Class#isSealed`. It cannot be overlook, can it?

There was always an ambiguity with the package java.lang.reflect, because it 
reflects the classfile (the .class) and not the source file (the .java).
So j.l.r.Modifier and j.l.r.AccessFlag reflects the modifiers of the classfile, 
see [1].

The keyword sealed and non-sealed are modifiers in the source file but not in 
the classfile. Sealed is represented as the attribute PermittedSubclasses and 
non-sealed is just no modifier (you have to op-out when a hierarchy is sealed 
in the source file while for the classfile it's opt-in). That's why both sealed 
and non-sealed are not defined in Modifier.

regards,
Rémi

[1] 
https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/AccessFlag.html

> 
>> On 2 Jan 2024, at 14:38, Roger Riggs  wrote:
>> 
>> Hi Pavel,
>> 
>> yes, a PR would be next.
>> 
>> Happy New Year, Roger
>> 
>> On 1/2/24 7:08 AM, Pavel Rappo wrote:
>>> I assume the order for `sealed` and `non-sealed` has effectively been 
>>> decided by
>>> JLS: https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1
>>> 
>>> 8.1.1. Class Modifiers
>>> ...
>>>  ClassModifier:
>>> (one of)
>>> Annotation public protected private
>>> abstract static final sealed non-sealed strictfp
>>>  ...
>>> 
>>> If two or more (distinct) class modifiers appear in a class 
>>> declaration, then it
>>> is customary, though not required, that they appear in the order 
>>> consistent
>>> with that shown above in the production for ClassModifier.
>>> 
>>> 
>>> Shall I just create a PR?
>>> 
 On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:
 
 I couldn't find any prior discussions on this matter.
 
 I noticed that bin/blessed-modifier-order.sh has not been updated for the
 [recently introduced](https://openjdk.org/jeps/409) `sealed` and 
 `non-sealed`
 keywords. I also note that we already have cases in OpenJDK where those
 keywords are ordered differently. If we have a consensus on how to extend 
 the
 "blessed order" onto those new keywords, I can create a PR to update the
 script.
 
 -Pavel
 

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v16]

[Brian Burkhalter]

On Thu, 21 Dec 2023 17:29:16 GMT, Brian Burkhalter  wrote:

>> Sergey Tsypanov 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 20 additional 
>> commits since the last revision:
>> 
>>  - 8322292: Remove TransferToTrusted.checkedOutputStream()
>>  - 8320971: Adjust JavaDoc
>>  - Merge branch 'master' into 8320971
>>  - 8320971: Revert irrelevant changes
>>  - 8320971: Add more tests
>>  - 8320971: Fix JavaDoc
>>  - 8320971: Whitespaces
>>  - 8320971: Fix build
>>  - 8320971: Move IOStreams to com.sun.io
>>  - Merge branch 'master' into 8320971
>>  - ... and 10 more: https://git.openjdk.org/jdk/compare/aed73e30...84686bc6
>
> The test does not fail when run against the mainline (HEAD: Thu Dec 21 
> 15:20:01 2023 + UTC). As previously mentioned elsewhere, normally a 
> deterministic test should fail before the proposed source patch is applied, 
> and succeed thereafter.

> @bplb has been reviewing the test, I'll stay out of that and let Brian finish 
> his review.

I think that the test looks all right, but the copyright might need to be 
changed to `2023, 2024` instead of just `2023` as it is a new file. The source 
file does not need `2024` if no changes have been made in the last two days.

-

PR Comment: https://git.openjdk.org/jdk/pull/16879#issuecomment-1874366815

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v6]

[Eirik Bjørsnøs]

> Please review this test-only PR which adds test coverage for 
> `ZipFile.getEntry` under certain charset conditions. 
> 
> When `ZipFile.getEntry` is called for an entry which has the `Language 
> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
> unconditionally, even when a different charset was supplied to the `ZipFile` 
> constructor.
> 
> It turns out we do not have any testing for this particular case, as can be 
> verified by commenting out the following line of code in 
> `ZipFile.Source.getEntryPos`:
> 
> 
> //ZipCoder zc = zipCoderForPos(pos);
> ``` 
> 
> and then running `make test TEST="test/jdk/java/util/zip"`
> 
> The current test verifies that the correct ZipCoder is used by 
> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
> 
> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
> (accidentally?) fixed by 
> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
> worthwhile to add explicit testing for this case to avoid regressions.
> 
> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
> JUnit 5. The conversion and modernization of the code is done in the first 
> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
> required to verify the `Language encoding flag` condition for 
> `ZipFile.getEntry`.
> 
> Testing: Verified that the test indeed fails when 
> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
> suggested above.

Eirik Bjørsnøs has updated the pull request incrementally with one additional 
commit since the last revision:

  Fine tuning assertion messages for consistency between tests

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17207/files
  - new: https://git.openjdk.org/jdk/pull/17207/files/8ba24628..dfdfb5bf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17207=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=17207=04-05

  Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/17207.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17207/head:pull/17207

PR: https://git.openjdk.org/jdk/pull/17207

Re: Blessed modifier order does not include sealed/non-sealed

[Roger Riggs]

Hi Pavel,

I agree, the script should refer to the JLS.
Some people look to the javadoc but its not authoritative on the language.

Roger

On 1/2/24 12:18 PM, Pavel Rappo wrote:

Right, the language model (javax.lang.model) is a better target to refer to 
when considering _source_ files. However, java.lang.reflect.Modifier 
corresponds to a set returned by javax.lang.model.element.Element#getModifiers, 
not to a single instance of javax.lang.model.element.Modifier; the order of 
that set is unspecified.

So perhaps the right thing would be to directly refer to JLS from the script. 
What do you think?


On 2 Jan 2024, at 16:56, Roger Riggs  wrote:

Hi Pavel,

It better to look to javax.lang.model.element.Modifier for the language view of 
the class.

java.lang.reflect.Modifier covers the modifier flags as represented in the 
class file and defined in the JVMS.
* The values for the constants
* representing the modifiers are taken from the tables in sections
* {@jvms 4.1}, {@jvms 4.4}, {@jvms 4.5}, and {@jvms 4.7} of
* The Java Virtual Machine Specification.
Sealing is represented in the class file as a non-empty list of permitted 
classes. Hence the method of java.lang.Class.

Since java.lang.Modifier.toString is based on the flag bits from the class file, 
"sealed" would not appear in any string it generates.


It might be possible to inject a comment in the toString method similar to the 
comment about interface not being a true modifier and including a reference to 
the javax.lang.model.element.Modifier enum.

Roger


On 1/2/24 11:31 AM, Pavel Rappo wrote:

Hi Roger,

Happy New Year to you too!

Although it's a _somewhat_ separate issue, I found that the shell script refers 
to java.lang.reflect.Modifier#toString which does NOT mention either `sealed` 
or `non-sealed`. More precisely, the script refers to the JDK 8 version of that 
method, but [the 
method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int))
 hasn't changed since 2009 and states that:

...The modifier names are returned in an order consistent with the suggested 
modifier orderings given in sections 8.1.1, 8.3.1, 8.4.3, 8.8.3, and 9.1.1 of 
The Java Language Specification. The full modifier ordering used by this method 
is:

public protected private abstract static final transient volatile synchronized 
native strictfp interface

It does not seem like `sealed` and `non-sealed` are even modelled by 
java.lang.reflect.Modifier, although `sealed` is modelled by 
`java.lang.Class#isSealed`. It cannot be overlook, can it?



On 2 Jan 2024, at 14:38, Roger Riggs  wrote:

Hi Pavel,

yes, a PR would be next.

Happy New Year, Roger

On 1/2/24 7:08 AM, Pavel Rappo wrote:


I assume the order for `sealed` and `non-sealed` has effectively been decided 
by JLS: https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1

8.1.1. Class Modifiers
...
ClassModifier:
(one of)
Annotation public protected private
abstract static final sealed non-sealed strictfp
...

If two or more (distinct) class modifiers appear in a class declaration, then 
it is customary, though not required, that they appear in the order consistent 
with that shown above in the production for ClassModifier.


Shall I just create a PR?



On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:

I couldn't find any prior discussions on this matter.

I noticed that bin/blessed-modifier-order.sh has not been updated for the [recently 
introduced](https://openjdk.org/jeps/409) `sealed` and `non-sealed` keywords. I also note 
that we already have cases in OpenJDK where those keywords are ordered differently. If we 
have a consensus on how to extend the "blessed order" onto those new keywords, 
I can create a PR to update the script.

-Pavel

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v5]

[Eirik Bjørsnøs]

> Please review this test-only PR which adds test coverage for 
> `ZipFile.getEntry` under certain charset conditions. 
> 
> When `ZipFile.getEntry` is called for an entry which has the `Language 
> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
> unconditionally, even when a different charset was supplied to the `ZipFile` 
> constructor.
> 
> It turns out we do not have any testing for this particular case, as can be 
> verified by commenting out the following line of code in 
> `ZipFile.Source.getEntryPos`:
> 
> 
> //ZipCoder zc = zipCoderForPos(pos);
> ``` 
> 
> and then running `make test TEST="test/jdk/java/util/zip"`
> 
> The current test verifies that the correct ZipCoder is used by 
> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
> 
> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
> (accidentally?) fixed by 
> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
> worthwhile to add explicit testing for this case to avoid regressions.
> 
> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
> JUnit 5. The conversion and modernization of the code is done in the first 
> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
> required to verify the `Language encoding flag` condition for 
> `ZipFile.getEntry`.
> 
> Testing: Verified that the test indeed fails when 
> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
> suggested above.

Eirik Bjørsnøs has updated the pull request incrementally with one additional 
commit since the last revision:

  Update some assertion failure messages

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17207/files
  - new: https://git.openjdk.org/jdk/pull/17207/files/4f53308c..8ba24628

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17207=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=17207=03-04

  Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/17207.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17207/head:pull/17207

PR: https://git.openjdk.org/jdk/pull/17207

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v4]

[Eirik Bjørsnøs]

On Tue, 2 Jan 2024 16:43:29 GMT, Lance Andersen  wrote:

>> Eirik Bjørsnøs has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Add more cases for 'language encoding' bit set, opened with a different 
>> encoding
>
> test/jdk/java/util/zip/ZipCoding.java line 130:
> 
>> 128: assertEquals(name, e.getName());
>> 129: assertNull(e.getComment()); // No comment in the LOC header
>> 130: assertArrayEquals(ENTRY_DATA, zis.readAllBytes(), "ZipIS 
>> content doesn't match!");
> 
> Minor Nit, but perhaps changes the "ZipIS content..." message to make it 
> clearer as it is not what is clear what ZipIS is...

These messages were mostly carried over. I updated this one to "ZIP entry data 
does not match". Also updated a few other assertion messages.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17207#discussion_r1439654471

Re: Blessed modifier order does not include sealed/non-sealed

[Pavel Rappo]

Right, the language model (javax.lang.model) is a better target to refer to 
when considering _source_ files. However, java.lang.reflect.Modifier 
corresponds to a set returned by javax.lang.model.element.Element#getModifiers, 
not to a single instance of javax.lang.model.element.Modifier; the order of 
that set is unspecified.

So perhaps the right thing would be to directly refer to JLS from the script. 
What do you think?

> On 2 Jan 2024, at 16:56, Roger Riggs  wrote:
> 
> Hi Pavel,
> 
> It better to look to javax.lang.model.element.Modifier for the language view 
> of the class.
> 
> java.lang.reflect.Modifier covers the modifier flags as represented in the 
> class file and defined in the JVMS.
> * The values for the constants
> * representing the modifiers are taken from the tables in sections
> * {@jvms 4.1}, {@jvms 4.4}, {@jvms 4.5}, and {@jvms 4.7} of
> * The Java Virtual Machine Specification.
> Sealing is represented in the class file as a non-empty list of permitted 
> classes. Hence the method of java.lang.Class.
> 
> Since java.lang.Modifier.toString is based on the flag bits from the class 
> file, "sealed" would not appear in any string it generates.
> 
> 
> It might be possible to inject a comment in the toString method similar to 
> the comment about interface not being a true modifier and including a 
> reference to the javax.lang.model.element.Modifier enum.
> 
> Roger
> 
> 
> On 1/2/24 11:31 AM, Pavel Rappo wrote:
>> Hi Roger,
>> 
>> Happy New Year to you too!
>> 
>> Although it's a _somewhat_ separate issue, I found that the shell script 
>> refers to java.lang.reflect.Modifier#toString which does NOT mention either 
>> `sealed` or `non-sealed`. More precisely, the script refers to the JDK 8 
>> version of that method, but [the 
>> method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int))
>>  hasn't changed since 2009 and states that:
>> 
>> ...The modifier names are returned in an order consistent with the suggested 
>> modifier orderings given in sections 8.1.1, 8.3.1, 8.4.3, 8.8.3, and 9.1.1 
>> of The Java Language Specification. The full modifier ordering used by this 
>> method is:
>> 
>> public protected private abstract static final transient volatile 
>> synchronized native strictfp interface
>> 
>> It does not seem like `sealed` and `non-sealed` are even modelled by 
>> java.lang.reflect.Modifier, although `sealed` is modelled by 
>> `java.lang.Class#isSealed`. It cannot be overlook, can it?
>> 
>> 
>>> On 2 Jan 2024, at 14:38, Roger Riggs  wrote:
>>> 
>>> Hi Pavel,
>>> 
>>> yes, a PR would be next.
>>> 
>>> Happy New Year, Roger
>>> 
>>> On 1/2/24 7:08 AM, Pavel Rappo wrote:
>>> 
 I assume the order for `sealed` and `non-sealed` has effectively been 
 decided by JLS: 
 https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1
 
 8.1.1. Class Modifiers
 ...
 ClassModifier:
 (one of)
 Annotation public protected private
 abstract static final sealed non-sealed strictfp
 ...
 
 If two or more (distinct) class modifiers appear in a class declaration, 
 then it is customary, though not required, that they appear in the order 
 consistent with that shown above in the production for ClassModifier.
 
 
 Shall I just create a PR?
 
 
> On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:
> 
> I couldn't find any prior discussions on this matter.
> 
> I noticed that bin/blessed-modifier-order.sh has not been updated for the 
> [recently introduced](https://openjdk.org/jeps/409) `sealed` and 
> `non-sealed` keywords. I also note that we already have cases in OpenJDK 
> where those keywords are ordered differently. If we have a consensus on 
> how to extend the "blessed order" onto those new keywords, I can create a 
> PR to update the script.
> 
> -Pavel
> 
> 
>>> 
>> 
> 

Re: Blessed modifier order does not include sealed/non-sealed

[Roger Riggs]

Hi Pavel,

It better to look to javax.lang.model.element.Modifier 
 
for the language view of the class.


java.lang.reflect.Modifier covers the modifier flags as represented in 
the class file and defined in the JVMS.


* The values for the constants * representing the modifiers are taken 
from the tables in sections * {@jvms 4.1}, {@jvms 4.4}, {@jvms 4.5}, and 
{@jvms 4.7} of * The Java Virtual Machine Specification.


Sealing is represented in the class file as a non-empty list of 
permitted classes. Hence the method of java.lang.Class.


Since java.lang.Modifier.toString is based on the flag bits from the 
class file, "sealed" would not appear in any string it generates.



It might be possible to inject a comment in the toString method similar 
to the comment about interface not being a true modifier and including a 
reference to the javax.lang.model.element.Modifier enum.


Roger


On 1/2/24 11:31 AM, Pavel Rappo wrote:

Hi Roger,

Happy New Year to you too!

Although it's a _somewhat_ separate issue, I found that the shell script refers 
to java.lang.reflect.Modifier#toString which does NOT mention either `sealed` 
or `non-sealed`. More precisely, the script refers to the JDK 8 version of that 
method, but [the 
method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int))
  hasn't changed since 2009 and states that:

 ...The modifier names are returned in an order consistent with the 
suggested modifier orderings given in sections 8.1.1, 8.3.1, 8.4.3, 8.8.3, and 
9.1.1 of The Java Language Specification. The full modifier ordering used by 
this method is:

 public protected private abstract static final transient volatile 
synchronized native strictfp interface

It does not seem like `sealed` and `non-sealed` are even modelled by 
java.lang.reflect.Modifier, although `sealed` is modelled by 
`java.lang.Class#isSealed`. It cannot be overlook, can it?


On 2 Jan 2024, at 14:38, Roger Riggs  wrote:

Hi Pavel,

yes, a PR would be next.

Happy New Year, Roger

On 1/2/24 7:08 AM, Pavel Rappo wrote:

I assume the order for `sealed` and `non-sealed` has effectively been decided 
by JLS:https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1

 8.1.1. Class Modifiers
 ...
  ClassModifier:
 (one of)
 Annotation public protected private
 abstract static final sealed non-sealed strictfp
  ...

 If two or more (distinct) class modifiers appear in a class declaration, 
then it is customary, though not required, that they appear in the order 
consistent with that shown above in the production for ClassModifier.


Shall I just create a PR?


On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:

I couldn't find any prior discussions on this matter.

I noticed that bin/blessed-modifier-order.sh has not been updated for the [recently 
introduced](https://openjdk.org/jeps/409) `sealed` and `non-sealed` keywords. I also note 
that we already have cases in OpenJDK where those keywords are ordered differently. If we 
have a consensus on how to extend the "blessed order" onto those new keywords, 
I can create a PR to update the script.

-Pavel

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v4]

[Lance Andersen]

On Tue, 2 Jan 2024 09:31:16 GMT, Eirik Bjørsnøs  wrote:

>> Please review this test-only PR which adds test coverage for 
>> `ZipFile.getEntry` under certain charset conditions. 
>> 
>> When `ZipFile.getEntry` is called for an entry which has the `Language 
>> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
>> unconditionally, even when a different charset was supplied to the `ZipFile` 
>> constructor.
>> 
>> It turns out we do not have any testing for this particular case, as can be 
>> verified by commenting out the following line of code in 
>> `ZipFile.Source.getEntryPos`:
>> 
>> 
>> //ZipCoder zc = zipCoderForPos(pos);
>> ``` 
>> 
>> and then running `make test TEST="test/jdk/java/util/zip"`
>> 
>> The current test verifies that the correct ZipCoder is used by 
>> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
>> 
>> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
>> (accidentally?) fixed by 
>> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
>> worthwhile to add explicit testing for this case to avoid regressions.
>> 
>> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
>> JUnit 5. The conversion and modernization of the code is done in the first 
>> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
>> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
>> required to verify the `Language encoding flag` condition for 
>> `ZipFile.getEntry`.
>> 
>> Testing: Verified that the test indeed fails when 
>> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
>> suggested above.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add more cases for 'language encoding' bit set, opened with a different 
> encoding

Looks good over all.  One minor comment to consider before pushing

test/jdk/java/util/zip/ZipCoding.java line 130:

> 128: assertEquals(name, e.getName());
> 129: assertNull(e.getComment()); // No comment in the LOC header
> 130: assertArrayEquals(ENTRY_DATA, zis.readAllBytes(), "ZipIS 
> content doesn't match!");

Minor Nit, but perhaps changes the "ZipIS content..." message to make it 
clearer as it is not what is clear what ZipIS is...

-

Marked as reviewed by lancea (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1800709897
PR Review Comment: https://git.openjdk.org/jdk/pull/17207#discussion_r1439626614

Re: Blessed modifier order does not include sealed/non-sealed

[Pavel Rappo]

Hi Roger,

Happy New Year to you too!

Although it's a _somewhat_ separate issue, I found that the shell script refers 
to java.lang.reflect.Modifier#toString which does NOT mention either `sealed` 
or `non-sealed`. More precisely, the script refers to the JDK 8 version of that 
method, but [the 
method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int))
 hasn't changed since 2009 and states that:

...The modifier names are returned in an order consistent with the 
suggested modifier orderings given in sections 8.1.1, 8.3.1, 8.4.3, 8.8.3, and 
9.1.1 of The Java Language Specification. The full modifier ordering used by 
this method is:

public protected private abstract static final transient volatile 
synchronized native strictfp interface

It does not seem like `sealed` and `non-sealed` are even modelled by 
java.lang.reflect.Modifier, although `sealed` is modelled by 
`java.lang.Class#isSealed`. It cannot be overlook, can it?

> On 2 Jan 2024, at 14:38, Roger Riggs  wrote:
> 
> Hi Pavel,
> 
> yes, a PR would be next.
> 
> Happy New Year, Roger
> 
> On 1/2/24 7:08 AM, Pavel Rappo wrote:
>> I assume the order for `sealed` and `non-sealed` has effectively been 
>> decided by JLS: 
>> https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1
>> 
>> 8.1.1. Class Modifiers
>> ...
>>  ClassModifier:
>> (one of)
>> Annotation public protected private
>> abstract static final sealed non-sealed strictfp
>>  ...
>> 
>> If two or more (distinct) class modifiers appear in a class declaration, 
>> then it is customary, though not required, that they appear in the order 
>> consistent with that shown above in the production for ClassModifier.
>> 
>> 
>> Shall I just create a PR?
>> 
>>> On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:
>>> 
>>> I couldn't find any prior discussions on this matter.
>>> 
>>> I noticed that bin/blessed-modifier-order.sh has not been updated for the 
>>> [recently introduced](https://openjdk.org/jeps/409) `sealed` and 
>>> `non-sealed` keywords. I also note that we already have cases in OpenJDK 
>>> where those keywords are ordered differently. If we have a consensus on how 
>>> to extend the "blessed order" onto those new keywords, I can create a PR to 
>>> update the script.
>>> 
>>> -Pavel
>>> 
> 

Re: RFR: 8322027: One XMLStreamException constructor fails to initialize cause

[Archie Cobbs]

On Tue, 2 Jan 2024 06:47:52 GMT, Jaikiran Pai  wrote:

>> One of the three `XMLStreamException` constructors that takes a `Throwable` 
>> fails to pass it to the superclass constructor.
>> 
>> This simple patch fixes that omission.
>> 
>> It's worth considering if there is any code out there that is working around 
>> this problem already by invoking `initCause()` manually. If so, that code 
>> would start throwing an `IllegalStateException` after this change.
>> 
>> So a more conservative fix would be to just add another constructor taking 
>> the same arguments in a different order. But then again that's not much 
>> better than just saying "always use initCause() with the broken 
>> constructor", i.e., don't change anything.
>> 
>> Hmm.
>
> Hello Archie, the changes look fine to me. I see that Joe has approved this 
> change and the CSR too has been approved. I've triggered a CI run to verify 
> there's no unexpected issues. Once that completes, I'll go ahead and sponsor 
> this.

@jaikiran - thanks for the review!

-

PR Comment: https://git.openjdk.org/jdk/pull/17090#issuecomment-1874251561

Re: RFR: 8322846: Running with -Djdk.tracePinnedThreads set can hang

[Alan Bateman]

On Tue, 2 Jan 2024 15:56:06 GMT, Jaikiran Pai  wrote:

> I guess that then means that the some pinned threads might not even appear in 
> this generated thread dumps.

Just to be clear. The changes here have no impact on thread dumps, the thread 
dump generated by jcmd Thread.dump_to_file has all virtual threads, including 
pinned threads.

As regards jdk.tracePinnedThreads. We want this system property to go away, 
it's just unfortunate that it seems to be widely used.

-

PR Comment: https://git.openjdk.org/jdk/pull/17221#issuecomment-1874211812

Re: RFR: 8322846: Running with -Djdk.tracePinnedThreads set can hang

[Jaikiran Pai]

On Tue, 2 Jan 2024 13:53:39 GMT, Alan Bateman  wrote:

> -Djdk.tracePinnedThreads is a debugging option that dates from early 
> development in the loom repo to identify pinned threads. It has several 
> issues and this tracing option will eventually be removed (use JFR events 
> instead). Several hangs have been reported when running with the system 
> property set. The "hangs" stem from the onPinned callback executing while the 
> virtual thread is in a transition state (typically parking). If the virtual 
> parks while printing the stack trace then it works like a nested park where 
> the thread state is never restored. Contention on the System.out can also 
> lead to deadlock when there are platform and pinned virtual threads printing 
> to System.out around the same time.
> 
> This PR brings over the changes from the loom repo to avoid these hangs. The 
> changes mean the stack trace is only printed to System.out when the 
> PrintStream lock can be acquired without blocking. It also restores the 
> thread state after printing.

Hello Alan, I haven't yet looked in detail the changes, so this merely a 
comment based on first glance:

> The changes mean the stack trace is only printed to System.out when the 
> PrintStream lock can be acquired without blocking.

I guess that then means that the some pinned threads might not even appear in 
this generated thread dumps. I can't think of a way where we could at least 
print something that would at least hint that some pinned threads weren't 
dumped in the output. This reduces the utility of this system property which is 
unfortunate, but I do understand the reason.

-

PR Comment: https://git.openjdk.org/jdk/pull/17221#issuecomment-1874208143

Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v19]

[Alan Bateman]

On Sat, 30 Dec 2023 16:47:11 GMT, Sergey Tsypanov  wrote:

>> It looks like we can skip copying of `byte[]` in 
>> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in 
>> `java.io`.
>> 
>> See comment by @vlsi in 
>> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612
>
> Sergey Tsypanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8320971: Fix JavaDoc

Implementation changes looks fine, that's for taking all the feedback to get 
this one to a good place.

@bplb has been reviewing the test, I'll stay out of that and let Brian finish 
his review.

-

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16879#pullrequestreview-1800627205

Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v3]

[Stefan Karlsson]

On Mon, 11 Dec 2023 14:06:43 GMT, Stefan Karlsson  wrote:

>> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename 
>> createJavaProcessBuilder' changed the name of the ProcessTools helper 
>> functions used to create `ProcessBuilder`s used to spawn new java test 
>> processes.
>> 
>> We now have `createTestJavaProcessBuilder` and 
>> `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, 
>> while the latter doesn't.
>> 
>> With these functions it is common to see the following pattern in tests:
>> 
>> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
>> OutputAnalyzer output = executeProcess(pb);
>> 
>> 
>> We have a couple of thin wrapper in `ProcessTools` that does exactly this, 
>> so that the code can be written as a one-liner:
>> 
>> OutputAnalyzer output = ProcessTools.executeTestJvm();
>> 
>> 
>> I propose that we name this functions using the same naming scheme we used 
>> for `createTestJavaProcessBuilder` and 
>> `createLimitedTestJavaProcessBuilder`. That is, we change `executeTestJvm` 
>> to `executeTestJava` and add a new `executeLimitedTestJava` function.
>
> Stefan Karlsson has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Test cleanup

Thanks for the reviews!

-

PR Comment: https://git.openjdk.org/jdk/pull/17049#issuecomment-1874176578

Re: RFR: 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder [v4]

[Stefan Karlsson]

> [JDK-8315097](https://bugs.openjdk.org/browse/JDK-8315097): 'Rename 
> createJavaProcessBuilder' changed the name of the ProcessTools helper 
> functions used to create `ProcessBuilder`s used to spawn new java test 
> processes.
> 
> We now have `createTestJavaProcessBuilder` and 
> `createLimitedTestJavaProcess`. The former prepends jvm options from jtreg, 
> while the latter doesn't.
> 
> With these functions it is common to see the following pattern in tests:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = executeProcess(pb);
> 
> 
> We have a couple of thin wrapper in `ProcessTools` that does exactly this, so 
> that the code can be written as a one-liner:
> 
> OutputAnalyzer output = ProcessTools.executeTestJvm();
> 
> 
> I propose that we name this functions using the same naming scheme we used 
> for `createTestJavaProcessBuilder` and `createLimitedTestJavaProcessBuilder`. 
> That is, we change `executeTestJvm` to `executeTestJava` and add a new 
> `executeLimitedTestJava` function.

Stefan Karlsson has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains four additional 
commits since the last revision:

 - Merge remote-tracking branch 'upstream/master' into rename_executeTestJvm
 - Test cleanup
 - Fix impl and add test
 - 8321713: Harmonize executeTestJvm with create[Limited]TestJavaProcessBuilder

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17049/files
  - new: https://git.openjdk.org/jdk/pull/17049/files/5d488f42..486dc6d5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17049=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=17049=02-03

  Stats: 5249 lines in 348 files changed: 3069 ins; 973 del; 1207 mod
  Patch: https://git.openjdk.org/jdk/pull/17049.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17049/head:pull/17049

PR: https://git.openjdk.org/jdk/pull/17049

Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]

[Stefan Karlsson]

On Tue, 12 Dec 2023 09:01:08 GMT, Stefan Karlsson  wrote:

>> There is some logging printed when tests spawns processes. This logging is 
>> triggered from calls to `OutputAnalyzer`, when it delegates calls to 
>> `LazyOutputBuffer`.
>> 
>> If we write code like this:
>> 
>> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
>> OutputAnalyzer output = new OutputAnalyzer(pb.start());
>> int exitValue = output.getExitValue();
>> 
>> 
>> We get the following logging:
>> 
>> [timestamp0] "Gathering output for process 
>> [timestamp1] Waiting for completion for process 
>> [timestamp2] Waiting for completion finished for process 
>> 
>> 
>> The first line comes from the `OutputAnalyzer` constructor and the two other 
>> lines comes from the `getExitValue` call, which calls logs the above 
>> messages around the call to `waitFor`.
>> 
>> If instead write the code above as:
>> 
>> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
>> OutputAnalyzer output = ProcessTools.executeProcess(pb);
>> int exitValue = output.getExitValue();
>> 
>> 
>> We get the same logging, but timestamp1 and timestamp2 are almost the same. 
>> This happens because there's an extra call to `waitFor` inside 
>> `executeProcess`, but that `waitFor` does not have the "wait for" logging. 
>> So, instead we get the logging for the no-op `waitFor` inside `getExitValue`.
>> 
>> I would like to propose a small workaround to solve this. The workaround is 
>> that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` 
>> via `OutputAnalyzer`.  This is a small, limited workaround for this issue. 
>> Ideally I would like to extract the entire Process handling code out of 
>> LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all 
>> usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the 
>> test directories, so I'm starting out with this suggestion.
>> 
>> We can see of this patch by looking at the timestamps generated from the 
>> included test. Without the proposed workaround:
>> 
>> Without
>> 
>> testExecuteProcessExit
>> [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719
>> [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719
>> [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process 
>> 2547719
>> 
>> testExecuteProcessStdout
>> [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741
>> [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741
>> [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process 
>> 2547741
>> 
>> 
>> testNewOutp...
>
> Stefan Karlsson has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Typo

Thanks for reviewing! I'll remove the test, merge, and will do some sanity 
checks before integrating.

-

PR Comment: https://git.openjdk.org/jdk/pull/17052#issuecomment-1874162036

Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v3]

[Stefan Karlsson]

> There is some logging printed when tests spawns processes. This logging is 
> triggered from calls to `OutputAnalyzer`, when it delegates calls to 
> `LazyOutputBuffer`.
> 
> If we write code like this:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = new OutputAnalyzer(pb.start());
> int exitValue = output.getExitValue();
> 
> 
> We get the following logging:
> 
> [timestamp0] "Gathering output for process 
> [timestamp1] Waiting for completion for process 
> [timestamp2] Waiting for completion finished for process 
> 
> 
> The first line comes from the `OutputAnalyzer` constructor and the two other 
> lines comes from the `getExitValue` call, which calls logs the above messages 
> around the call to `waitFor`.
> 
> If instead write the code above as:
> 
> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...);
> OutputAnalyzer output = ProcessTools.executeProcess(pb);
> int exitValue = output.getExitValue();
> 
> 
> We get the same logging, but timestamp1 and timestamp2 are almost the same. 
> This happens because there's an extra call to `waitFor` inside 
> `executeProcess`, but that `waitFor` does not have the "wait for" logging. 
> So, instead we get the logging for the no-op `waitFor` inside `getExitValue`.
> 
> I would like to propose a small workaround to solve this. The workaround is 
> that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` 
> via `OutputAnalyzer`.  This is a small, limited workaround for this issue. 
> Ideally I would like to extract the entire Process handling code out of 
> LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all 
> usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the 
> test directories, so I'm starting out with this suggestion.
> 
> We can see of this patch by looking at the timestamps generated from the 
> included test. Without the proposed workaround:
> 
> Without
> 
> testExecuteProcessExit
> [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719
> [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719
> [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process 
> 2547719
> 
> testExecuteProcessStdout
> [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741
> [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741
> [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process 
> 2547741
> 
> 
> testNewOutputAnalyzerExit
> [2023-12-11T11:05:46.231475003Z] Gathering output for process 2547782
> [2023...

Stefan Karlsson has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains five additional 
commits since the last revision:

 - Merge remote-tracking branch 'upstream/master' into executeProcessLogging
 - Remove temporary test
 - Typo
 - Whitespace cleanups
 - 8321718: ProcessTools.executeProcess calls waitFor before logging

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17052/files
  - new: https://git.openjdk.org/jdk/pull/17052/files/3275136c..fcba9dbd

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17052=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=17052=01-02

  Stats: 5347 lines in 349 files changed: 3069 ins; 1071 del; 1207 mod
  Patch: https://git.openjdk.org/jdk/pull/17052.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17052/head:pull/17052

PR: https://git.openjdk.org/jdk/pull/17052

RFR: 8322818: Thread::getStackTrace can fail with InternalError if virtual thread is timed-parked when pinned

[Alan Bateman]

Missed by JDK-8312498, VirtualThread::tryGetStackTrace doesn't handle the 
TIMED_PINNED state so it's possible for Thread::getStackTrace to throw 
InternalError when invoked on a virtual thread that quickly transitions from 
unmounted to  timed-park-while-pinned. This one is needs a stress test to 
reproduce.

-

Commit messages:
 - No need to restrict stress test to VMContinuations
 - Initial commit

Changes: https://git.openjdk.org/jdk/pull/17217/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17217=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322818
  Stats: 78 lines in 2 files changed: 75 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/17217.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17217/head:pull/17217

PR: https://git.openjdk.org/jdk/pull/17217

RFR: 8322846: Running with -Djdk.tracePinnedThreads set can hang

[Alan Bateman]

-Djdk.tracePinnedThreads is a debugging option that dates from early 
development in the loom repo to identify pinned threads. It has several issues 
and this tracing option will eventually be removed (use JFR events instead). 
Several hangs have been reported when running with the system property set. The 
"hangs" stem from the onPinned callback executing while the virtual thread is 
in a transition state (typically parking). If the virtual parks while printing 
the stack trace then it works like a nested park where the thread state is 
never restored. Contention on the System.out can also lead to deadlock when 
there are platform and pinned virtual threads printing to System.out around the 
same time.

This PR brings over the changes from the loom repo to avoid these hangs. The 
changes mean the stack trace is only printed to System.out when the PrintStream 
lock can be acquired without blocking. It also restores the thread state after 
printing.

-

Commit messages:
 - Initial commit

Changes: https://git.openjdk.org/jdk/pull/17221/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17221=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322846
  Stats: 126 lines in 3 files changed: 98 ins; 12 del; 16 mod
  Patch: https://git.openjdk.org/jdk/pull/17221.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17221/head:pull/17221

PR: https://git.openjdk.org/jdk/pull/17221

RFR: 8322829: Refactor nioBlocker to avoid blocking while holding Thread's interrupt lock

[Alan Bateman]

In preparation for when virtual threads can unmount while holding a monitor or 
unmount when blocking on monitorenter, the implementation of VirtualThread's 
interrupt method is refactored to avoid parking/blocking while holding the 
Thread's interrupt lock. The implementations of sun.nio.ch.Interruptible are 
refactored to close/wakeup the InterruptibleChannel/Selector after releasing 
the interrupt lock. There is a lot of test coverage for async close and 
interrupt, no additional tests are added.

-

Commit messages:
 - Initial commit

Changes: https://git.openjdk.org/jdk/pull/17219/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17219=00
  Issue: https://bugs.openjdk.org/browse/JDK-8322829
  Stats: 141 lines in 6 files changed: 87 ins; 25 del; 29 mod
  Patch: https://git.openjdk.org/jdk/pull/17219.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17219/head:pull/17219

PR: https://git.openjdk.org/jdk/pull/17219

Re: RFR: 8320707: Virtual thread test updates [v2]

[Alan Bateman]

> A lot of test changes have accumulated in the loom repo, this includes both 
> new tests and updates to existing tests. Some of these updates can be brought 
> to the main line. This update brings over:
> 
> - The existing tests for pinning use synchronized blocks. In preparation for 
> changes to allow carrier thread be released when a virtual thread parks 
> holding a monitor or blocks on monitorenter, these tests are changed to pin 
> by having a native frame on the stack. This part includes test infrastructure 
> to make it easy to add more tests that do operations while pinned. The tests 
> still test what they were originally created to test of course.
> 
> - The test for the JFR jdk.VirtualThreadPinned event is refactored to allow 
> for additional cases where the event may be reported.
> 
> - ThreadAPI is expanded to cover test for uncaught exception handling.
> 
> - GetStackTraceWhenRunnable is refactored to not use a Selector, otherwise 
> this test will be invalidated when blocking selection operations release the 
> carrier.
> 
> - StressStackOverflow is dialed down to run for 1m instead of 2mins.
> 
> - The use of CountDownLatch in a number of tests that poll thread state has 
> been dropped to keep the tests as simple as possible.

Alan Bateman has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains four additional commits since 
the last revision:

 - Revert changes to TracePinnedThreads.java
 - Sync up from loom repo
 - Merge
 - Initial commit

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17136/files
  - new: https://git.openjdk.org/jdk/pull/17136/files/b7abfc0a..fe255921

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17136=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=17136=00-01

  Stats: 3108 lines in 165 files changed: 1976 ins; 659 del; 473 mod
  Patch: https://git.openjdk.org/jdk/pull/17136.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17136/head:pull/17136

PR: https://git.openjdk.org/jdk/pull/17136

Re: RFR: 8311864: Add ArraysSupport.hashCode(int[] a, fromIndex, length, initialValue) [v4]

[Pavel Rappo]

> This PR adds an internal method to calculate hash code from the provided 
> integer array, offset and length into that array, and the initial hash code 
> value.
> 
> Current options for calculating a hash code for int[] are inflexible. It's 
> either ArraysSupport.vectorizedHashCode with an offset, length and initial 
> value, or Arrays.hashCode with just an array.
> 
> For an arbitrary int[], unconditional vectorization might be unwarranted or 
> puzzling. Unfortunately, it's the only hash code method that operates on an 
> array subrange or accepts the initial hash code value.
> 
> A more convenient method could be added and then used, for example, here:
> 
> * 
> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java#L1076-L1083
> 
> * 
> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/ArrayList.java#L669-L680
> 
> * 
> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/sun/security/pkcs10/PKCS10.java#L356-L362
> 
> This PR adds such a method and provides tests for it. Additionally, this PR 
> adds tests for `null` passed to `java.util.Arrays.hashCode` overloads, 
> behaviour which was specified but untested.

Pavel Rappo has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains four additional commits since 
the last revision:

 - Merge branch 'master' into 8311864
 - Merge remote-tracking branch 'jdk/master' into 8311864
 - Merge branch 'master' into 8311864
 - Initial commit

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14831/files
  - new: https://git.openjdk.org/jdk/pull/14831/files/655442eb..e55dc5c1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14831=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=14831=02-03

  Stats: 4826 lines in 316 files changed: 2898 ins; 812 del; 1116 mod
  Patch: https://git.openjdk.org/jdk/pull/14831.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14831/head:pull/14831

PR: https://git.openjdk.org/jdk/pull/14831

Re: Blessed modifier order does not include sealed/non-sealed

[Roger Riggs]

Hi Pavel,

yes, a PR would be next.

Happy New Year, Roger

On 1/2/24 7:08 AM, Pavel Rappo wrote:

I assume the order for `sealed` and `non-sealed` has effectively been decided 
by JLS: https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1

 8.1.1. Class Modifiers
 ...
 
 ClassModifier:

 (one of)
 Annotation public protected private
 abstract static final sealed non-sealed strictfp
 
 ...


 If two or more (distinct) class modifiers appear in a class declaration, 
then it is customary, though not required, that they appear in the order 
consistent with that shown above in the production for ClassModifier.


Shall I just create a PR?


On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:

I couldn't find any prior discussions on this matter.

I noticed that bin/blessed-modifier-order.sh has not been updated for the [recently 
introduced](https://openjdk.org/jeps/409) `sealed` and `non-sealed` keywords. I also note 
that we already have cases in OpenJDK where those keywords are ordered differently. If we 
have a consensus on how to extend the "blessed order" onto those new keywords, 
I can create a PR to update the script.

-Pavel

Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v10]

[Pavel Rappo]

> Please review this PR to use modern APIs and language features to simplify 
> equals, hashCode, and compareTo for BigInteger. If you have any performance 
> concerns, please raise them.
> 
> This PR is cherry-picked from a bigger, not-yet-published PR, to test the 
> waters. That latter PR will be published soon.

Pavel Rappo 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 17 additional commits since the 
last revision:

 - Merge branch 'master' into 8310813
 - Merge branch 'master' into 8310813
 - Merge branch 'master' into 8310813
 - Merge branch 'master' into 8310813
 - Fix bugs in Shared.createSingle
 - Merge branch 'master' into 8310813
 - Group params coarser (suggested by @cl4es)
   
   - Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge.
 Every testXYZ method invokes M operations, where M is the maximum
 number of elements in a group. Shorter groups are cyclically padded.
   - Uses the org.openjdk.jmh.infra.Blackhole API and increases
 benchmark time.
   - Fixes a bug in Shared that precluded 0 from being in a pair.
 - Use better overloads (suggested by @cl4es)
   
   - Uses simpler, more suitable overloads for the subrange
 starting from 0
 - Improve benchmarks
 - Merge branch 'master' into 8310813
 - ... and 7 more: https://git.openjdk.org/jdk/compare/cd913e32...252b7378

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14630/files
  - new: https://git.openjdk.org/jdk/pull/14630/files/ef8b0c46..252b7378

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14630=09
 - incr: https://webrevs.openjdk.org/?repo=jdk=14630=08-09

  Stats: 4826 lines in 316 files changed: 2898 ins; 812 del; 1116 mod
  Patch: https://git.openjdk.org/jdk/pull/14630.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14630/head:pull/14630

PR: https://git.openjdk.org/jdk/pull/14630

Re: RFR: JDK-8319122: Improve documentation of various Zip-file related APIs [v2]

[Yakov Shafranovich]

On Fri, 10 Nov 2023 15:44:19 GMT, Yakov Shafranovich  wrote:

>> The various Zip/Jar-file related Java APIs have some long-standing 
>> differences or peculiarities with respect to the ZIP-file specification or 
>> compared to other implementations which should be documented in the API-doc. 
>> This documents the following:
>> - Cache of JAR files in JarURLConnection class
>> - Cache of JAR/ZIP files in JarFile and ZipFile classes
>> - Unexpected behavior when parsing ZIP files with duplicate entries in 
>> JarFile and ZipFile classes, as well as the zipfs provider
>> - Directories and filenames with the same name considered to be the same in 
>> ZipFile class
>> - Possible issues when local and central headers conflict in ZipInputStream 
>> class
>> 
>> Related JBS report:
>> https://bugs.openjdk.org/browse/JDK-8319122
>
> Yakov Shafranovich has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Fixed more line breaks
>  - fixed line breaks

Still working on this

-

PR Comment: https://git.openjdk.org/jdk/pull/16424#issuecomment-1874073990

Re: RFR: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile [v9]

[Eirik Bjørsnøs]

> This PR suggests we retire the binary test vectors `ZipFile/input.zip`, 
> `ZipFile/input.jar` and `ZipFile/crash.jar`
> 
> Binary test vectors are harder to analyze,  and sharing test vectors across 
> unrelated tests increases maintenance burden. It would be better to have each 
> test produce its own test vectors independently.
> 
> While visiting these dusty tests, we should take the opportunity to convert 
> them to JUnit, add more comments and perform some mild modernization and 
> cleanups where appropriate. We should also consider whether any test are 
> duplicated and can be retired or moved into other test files as separate 
> methods. See comments below.
> 
> To help reviewers, here are some comments on the updated tests:
> 
> `Available.java`
> This test currently has no jtreg `@test` header, so isn't currently active. 
> After discussion, we decided to merge this test into `ReadZip.java`. I added 
> some checks to verify that reading from the stream reduces the number of 
> available bytes accordingly, also checking the behavior when the stream is 
> closed.
> 
> `CopyJar.java`
> The concern of copying entries seems to now have better coverage in the test 
> `zip/CopyZipFile`. We decided to retire this test rather than convert it and 
> instead convert `CopyZipFile` to JUnit.
> 
> `EnumAfterClose.java`
> To prevent confusion with Java Enums, I suggest we rename this test to 
> `EnumerateAfterClose`.
> 
> `FinalizeInflater.java`  
> The code verified by this test has been updated to use cleaners instead of 
> finalizers. Still, this test code relies on finalizers. Not sure if this is 
> an issue, but this test will need to be updated when finalizers are finally 
> removed.
> 
> `GetDirEntry.java`
> We decided to merge this test into `ReadZip.readDirectoryEntries` rather than 
> keeping it as a separate test.
> 
> `ReadZip.java`
> Nothing exciting here, the single main method was split into multiple JUnit 
> methods, each focusing on a separate concern. A new test case `noentries()` 
> was added, resolving 
> [JDK-8322830](https://bugs.openjdk.org/browse/JDK-8322830)
> 
> `ReleaseInflater.java`
> Nothing exciting, tried to add some comment to help understanding of what is 
> tested.
> 
> `StreamZipEntriesTest.java`
> This test was using TestNG so was converted to JUnit for consistency. Added 
> some comments to help understanding.
> 
> `ReadAfterClose.java`
> This uses the binary test vector `crash.jar`. The test is converted to JUnit 
> and moved to `ReadZip.readAfterClose´. The test is expanded to exercise more 
> read methods.

Eirik Bjørsnøs has updated the pull request incrementally with one additional 
commit since the last revision:

  Add test verifying that ZipFile can open a ZIP file with zero entries

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17038/files
  - new: https://git.openjdk.org/jdk/pull/17038/files/85500cd8..669b8da1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17038=08
 - incr: https://webrevs.openjdk.org/?repo=jdk=17038=07-08

  Stats: 30 lines in 1 file changed: 28 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/17038.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038

PR: https://git.openjdk.org/jdk/pull/17038

Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]

[Adam Sotona]

On Thu, 7 Dec 2023 07:53:58 GMT, Adam Sotona  wrote:

>> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is 
>> corrupted so the parser attempts to read beyond the classfile bounds.
>> General contract is that only `IllegalArgumentException` or its subclasses 
>> is expected when parser fails.
>> This patch wraps `IndexOutOfBoundsExceptions` thrown from all 
>> `ClassReaderImpl.buffer` manipulations into an  
>> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
>> Relevant tests are added.
>> 
>> Please review.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains three commits:
> 
>  - Merge branch 'master' into JDK-8320360-bounds
>
># Conflicts:
>#  test/jdk/jdk/classfile/LimitsTest.java
>  - added bug # to the test
>  - 8320360: ClassFile.parse: Some defect class files cause unexpected 
> exceptions to be thrown

Thanks for the review!

-

PR Comment: https://git.openjdk.org/jdk/pull/16762#issuecomment-1873994195

Integrated: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown

[Adam Sotona]

On Tue, 21 Nov 2023 13:59:23 GMT, Adam Sotona  wrote:

> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is 
> corrupted so the parser attempts to read beyond the classfile bounds.
> General contract is that only `IllegalArgumentException` or its subclasses is 
> expected when parser fails.
> This patch wraps `IndexOutOfBoundsExceptions` thrown from all 
> `ClassReaderImpl.buffer` manipulations into an  
> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
> Relevant tests are added.
> 
> Please review.
> 
> Thanks,
> Adam

This pull request has now been integrated.

Changeset: a5cf4210
Author:Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/a5cf4210cd9c293a9e9bce60dc6d0f08fd838c77
Stats: 71 lines in 2 files changed: 47 ins; 0 del; 24 mod

8320360: ClassFile.parse: Some defect class files cause unexpected exceptions 
to be thrown

Reviewed-by: jpai

-

PR: https://git.openjdk.org/jdk/pull/16762

Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]

[Adam Sotona]

On Tue, 2 Jan 2024 07:03:55 GMT, Jaikiran Pai  wrote:

>> Adam Sotona has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains three commits:
>> 
>>  - Merge branch 'master' into JDK-8320360-bounds
>>
>># Conflicts:
>># test/jdk/jdk/classfile/LimitsTest.java
>>  - added bug # to the test
>>  - 8320360: ClassFile.parse: Some defect class files cause unexpected 
>> exceptions to be thrown
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
> line 200:
> 
>> 198: try {
>> 199: return buffer[p] & 0xFF;
>> 200: } catch (IndexOutOfBoundsException e) {
> 
> This and all other catch blocks introduced in this change can be changed to 
> the specific exception type `ArrayIndexOutOfBoundsException`, because all 
> these operations are dealing with only array access. If you prefer catching 
> this more generic `IndexOutOfBoundsException`, that's fine with me.

Yes, I prefer the generic `IndexOutOfBoundsException`, thanks.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439426562

Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v3]

[Adam Sotona]

> ClassFile API throws `IndexOutOfBoundsException` when classfile structure is 
> corrupted so the parser attempts to read beyond the classfile bounds.
> General contract is that only `IllegalArgumentException` or its subclasses is 
> expected when parser fails.
> This patch wraps `IndexOutOfBoundsExceptions` thrown from all 
> `ClassReaderImpl.buffer` manipulations into an  
> `IllegalArgumentException("Reading beyond classfile bounds", iOOBECause)`.
> Relevant tests are added.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  updated copyright years

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16762/files
  - new: https://git.openjdk.org/jdk/pull/16762/files/08bcc548..a16bf6da

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=16762=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=16762=01-02

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/16762.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16762/head:pull/16762

PR: https://git.openjdk.org/jdk/pull/16762

Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]

[Adam Sotona]

On Tue, 2 Jan 2024 12:41:08 GMT, Adam Sotona  wrote:

>> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
>> line 283:
>> 
>>> 281: public void copyBytesTo(BufWriter buf, int p, int len) {
>>> 282: try {
>>> 283: buf.writeBytes(buffer, p, len);
>> 
>> `java.lang.classfile.BufWriter` doesn't specify any `@throws` for its 
>> `writeXXX` methods. Should it be specified (of course in a separate PR)?
>
> That is a good point, thanks!

I've created [JDK-8322847](https://bugs.openjdk.org/browse/JDK-8322847)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439418563

Re: RFR: 8320360: ClassFile.parse: Some defect class files cause unexpected exceptions to be thrown [v2]

[Adam Sotona]

On Tue, 2 Jan 2024 07:05:22 GMT, Jaikiran Pai  wrote:

>> Adam Sotona has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains three commits:
>> 
>>  - Merge branch 'master' into JDK-8320360-bounds
>>
>># Conflicts:
>># test/jdk/jdk/classfile/LimitsTest.java
>>  - added bug # to the test
>>  - 8320360: ClassFile.parse: Some defect class files cause unexpected 
>> exceptions to be thrown
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java 
> line 283:
> 
>> 281: public void copyBytesTo(BufWriter buf, int p, int len) {
>> 282: try {
>> 283: buf.writeBytes(buffer, p, len);
> 
> `java.lang.classfile.BufWriter` doesn't specify any `@throws` for its 
> `writeXXX` methods. Should it be specified (of course in a separate PR)?

That is a good point, thanks!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16762#discussion_r1439414618

Integrated: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures

[Adam Sotona]

On Mon, 11 Dec 2023 15:17:49 GMT, Adam Sotona  wrote:

> ClassFile API models and parses signatures, however parsing of the signatures 
> mostly throws IIOBE when it fails.
> This patch improves SignaturesImpl parsing methods implementation and errors 
> handling and adds relevant negative tests.
> The parser is not an ultimate signatures validator yet, however this is a 
> step forward to it.
> 
> Please review.
> 
> Thanks,
> Adam

This pull request has now been integrated.

Changeset: f9aec02f
Author:Adam Sotona 
URL:   
https://git.openjdk.org/jdk/commit/f9aec02f3caabb6bc06672c214127f8912449615
Stats: 136 lines in 2 files changed: 103 ins; 5 del; 28 mod

8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for 
invalid signatures

Reviewed-by: jpai

-

PR: https://git.openjdk.org/jdk/pull/17058

Re: RFR: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures [v2]

[Adam Sotona]

On Tue, 2 Jan 2024 07:17:46 GMT, Jaikiran Pai  wrote:

> I'm guessing you have intentionally not included the original 
> `IndexOutOfBoundsException` as a cause in the `IllegalArgumentException` that 
> gets thrown.

Yes, original exception does not contain any meaningful information.

> Also, it appears that the `SignaturesImpl` is more like an utility class and 
> isn't expected to be used in multi-thread access and thus the reliance on the 
> (non final) instance field `sig` appears OK.

Right, `SignaturesImpl` instance is never exposed for multi-threaded access.

Thanks for the review!

-

PR Comment: https://git.openjdk.org/jdk/pull/17058#issuecomment-1873974280

Re: RFR: 8321540: ClassSignature.parseFrom() throws StringIndexOutOfBoundsException for invalid signatures [v2]

[Adam Sotona]

> ClassFile API models and parses signatures, however parsing of the signatures 
> mostly throws IIOBE when it fails.
> This patch improves SignaturesImpl parsing methods implementation and errors 
> handling and adds relevant negative tests.
> The parser is not an ultimate signatures validator yet, however this is a 
> step forward to it.
> 
> Please review.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  updated copyright year

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17058/files
  - new: https://git.openjdk.org/jdk/pull/17058/files/da060bbb..e856dffe

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17058=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=17058=00-01

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/17058.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17058/head:pull/17058

PR: https://git.openjdk.org/jdk/pull/17058

Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v2]

[Eirik Bjørsnøs]

On Tue, 2 Jan 2024 10:37:55 GMT, Jaikiran Pai  wrote:

>> Eirik Bjørsnøs has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Prevent IOException thrown during finish() from being lost if an 
>> IOException is thrown while closing the wrapped stream
>
> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 257:
> 
>> 255: } catch (IOException ioe) {
>> 256: if (finishException != ioe) {
>> 257: ioe.addSuppressed(finishException);
> 
> A null check for `finishException` will be needed here to prevent a 
> `NullPointerException` being thrown from within `addSuppressed`. I think it's 
> better to just copy over (rest of) the finally block from 
> `FilterOutputStream`'s close() method.

Updated the code to align with `FilterOutputStream.close()`:

- Avoid the suppression try/catch for the case where finishException is null
- Move `closed = true;` to the top of the method, this makes sure the wrapped 
stream is only closed once even in the case where it throws during close. Close 
should only be attempted once.

I added more test cases including one where both finish and close operations 
fail, one where finish and close operations throw identical IOException 
instances and one testing that the wrapped stream is closed only once, even if 
the close operation throws.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1439403755

Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v3]

[Eirik Bjørsnøs]

> Please consider this PR which makes `DeflaterOutputStream.close()` always 
> close its wrapped output stream.
> 
> Currently, closing of the wrapped output stream happens outside the finally 
> block where `finish()` is called. If `finish()` throws, this means the 
> wrapped stream will not be closed. This can potentially lead to leaking 
> resources such as file descriptors or sockets.
> 
> This fix is to move the closing of the wrapped stream inside the finally 
> block.
> 
> Specification: This change brings the implementation of 
> `DeflaterOutputStream.close()` in line with its specification:  *Writes 
> remaining compressed data to the output stream and closes the underlying 
> stream.*
> 
> Risk: This is a behavioural change. There is a small risk that existing code 
> depends on the close method not following its specification.
> 
> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which 
> simulates the failure condition and verifies that the wrapped stream was 
> closed under failing and non-failing conditions.

Eirik Bjørsnøs has updated the pull request incrementally with two additional 
commits since the last revision:

 - Add more extensive testing of combined write/close failure modes
 - Don't suppress if finishException is null, mark stream as closed even when 
closing the wrapped stream failed

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17209/files
  - new: https://git.openjdk.org/jdk/pull/17209/files/02707d58..33e7756e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17209=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=17209=01-02

  Stats: 149 lines in 2 files changed: 123 ins; 2 del; 24 mod
  Patch: https://git.openjdk.org/jdk/pull/17209.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17209/head:pull/17209

PR: https://git.openjdk.org/jdk/pull/17209

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v4]

[Alan Bateman]

On Tue, 2 Jan 2024 09:31:16 GMT, Eirik Bjørsnøs  wrote:

>> Please review this test-only PR which adds test coverage for 
>> `ZipFile.getEntry` under certain charset conditions. 
>> 
>> When `ZipFile.getEntry` is called for an entry which has the `Language 
>> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
>> unconditionally, even when a different charset was supplied to the `ZipFile` 
>> constructor.
>> 
>> It turns out we do not have any testing for this particular case, as can be 
>> verified by commenting out the following line of code in 
>> `ZipFile.Source.getEntryPos`:
>> 
>> 
>> //ZipCoder zc = zipCoderForPos(pos);
>> ``` 
>> 
>> and then running `make test TEST="test/jdk/java/util/zip"`
>> 
>> The current test verifies that the correct ZipCoder is used by 
>> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
>> 
>> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
>> (accidentally?) fixed by 
>> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
>> worthwhile to add explicit testing for this case to avoid regressions.
>> 
>> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
>> JUnit 5. The conversion and modernization of the code is done in the first 
>> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
>> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
>> required to verify the `Language encoding flag` condition for 
>> `ZipFile.getEntry`.
>> 
>> Testing: Verified that the test indeed fails when 
>> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
>> suggested above.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add more cases for 'language encoding' bit set, opened with a different 
> encoding

> The change to allow user/application specific arbritary charsets to the 
> `ZipFile` constructor seems to have been done long back in Java 1.7 days as 
> part of JDK-4244499.

There is a lot of history in this area. ZIP dates from the days of MS-DOS where 
it used IBM 437 for encoding the names of entries. So different to Java where 
it uses UTF-8 for JAR files and also non-JAR ZIP files. Up to this point (as in 
JDK 7) there were also issues with the UTF-8 decoding and some forms of 
supplementary characters. Sherman got things to a good place in JDK 7 and also 
added the constructors so you can specify the encoding when you obtain it from 
some out of band means.

-

PR Comment: https://git.openjdk.org/jdk/pull/17207#issuecomment-1873950701

Re: Blessed modifier order does not include sealed/non-sealed

[Remi Forax]

- Original Message -
> From: "Pavel Rappo" 
> To: "core-libs-dev" 
> Sent: Tuesday, January 2, 2024 12:56:08 PM
> Subject: Blessed modifier order does not include sealed/non-sealed

> I couldn't find any prior discussions on this matter.
> 
> I noticed that bin/blessed-modifier-order.sh has not been updated for the
> [recently introduced](https://openjdk.org/jeps/409) `sealed` and `non-sealed`
> keywords. I also note that we already have cases in OpenJDK where those
> keywords are ordered differently. If we have a consensus on how to extend the
> "blessed order" onto those new keywords, I can create a PR to update the
> script.
> 
> -Pavel

[amber-spec-experts added]

Hello,
For me, sealed, non-sealed and final are (mutually exclusive) modifiers that 
control of the subtypes of a class.
Given that there is already a blessed order for final, sealed and non-sealed 
should be at the same place as final.

regards,
Rémi

Re: Blessed modifier order does not include sealed/non-sealed

[Pavel Rappo]

I assume the order for `sealed` and `non-sealed` has effectively been decided 
by JLS: https://docs.oracle.com/javase/specs/jls/se21/html/jls-8.html#jls-8.1.1

8.1.1. Class Modifiers
...

ClassModifier:
(one of)
Annotation public protected private
abstract static final sealed non-sealed strictfp

...

If two or more (distinct) class modifiers appear in a class declaration, 
then it is customary, though not required, that they appear in the order 
consistent with that shown above in the production for ClassModifier.


Shall I just create a PR?

> On 2 Jan 2024, at 11:56, Pavel Rappo  wrote:
> 
> I couldn't find any prior discussions on this matter.
> 
> I noticed that bin/blessed-modifier-order.sh has not been updated for the 
> [recently introduced](https://openjdk.org/jeps/409) `sealed` and `non-sealed` 
> keywords. I also note that we already have cases in OpenJDK where those 
> keywords are ordered differently. If we have a consensus on how to extend the 
> "blessed order" onto those new keywords, I can create a PR to update the 
> script.
> 
> -Pavel
> 

Blessed modifier order does not include sealed/non-sealed

[Pavel Rappo]

I couldn't find any prior discussions on this matter.

I noticed that bin/blessed-modifier-order.sh has not been updated for the 
[recently introduced](https://openjdk.org/jeps/409) `sealed` and `non-sealed` 
keywords. I also note that we already have cases in OpenJDK where those 
keywords are ordered differently. If we have a consensus on how to extend the 
"blessed order" onto those new keywords, I can create a PR to update the script.

-Pavel

Integrated: 8317846: Typo in API documentation of classes IdentityHashMap

[ANUPAM DEV]

On Wed, 11 Oct 2023 04:45:20 GMT, ANUPAM DEV  wrote:

> Hello,
> 
> I have fixed the typo in the comment for the constructor 
> IdentityHashMap(Map m)
> 
> before: Constructs a new identity hash map containing the keys-value mappings 
> in the specified map.
> after: Constructs a new identity hash map containing the key-value mappings 
> in the specified map.
> 
> Please review the change.
> 
> Regards,
> Anupam

This pull request has now been integrated.

Changeset: d4fb3088
Author:ANUPAM DEV 
Committer: Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/d4fb30885b007baab243536458a54b6ade610218
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8317846: Typo in API documentation of classes IdentityHashMap

Reviewed-by: mli, jpai

-

PR: https://git.openjdk.org/jdk/pull/16134

Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v2]

[Jaikiran Pai]

On Tue, 2 Jan 2024 09:14:14 GMT, Eirik Bjørsnøs  wrote:

>> Please consider this PR which makes `DeflaterOutputStream.close()` always 
>> close its wrapped output stream.
>> 
>> Currently, closing of the wrapped output stream happens outside the finally 
>> block where `finish()` is called. If `finish()` throws, this means the 
>> wrapped stream will not be closed. This can potentially lead to leaking 
>> resources such as file descriptors or sockets.
>> 
>> This fix is to move the closing of the wrapped stream inside the finally 
>> block.
>> 
>> Specification: This change brings the implementation of 
>> `DeflaterOutputStream.close()` in line with its specification:  *Writes 
>> remaining compressed data to the output stream and closes the underlying 
>> stream.*
>> 
>> Risk: This is a behavioural change. There is a small risk that existing code 
>> depends on the close method not following its specification.
>> 
>> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which 
>> simulates the failure condition and verifies that the wrapped stream was 
>> closed under failing and non-failing conditions.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Prevent IOException thrown during finish() from being lost if an 
> IOException is thrown while closing the wrapped stream

src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 257:

> 255: } catch (IOException ioe) {
> 256: if (finishException != ioe) {
> 257: ioe.addSuppressed(finishException);

A null check for `finishException` will be needed here to prevent a 
`NullPointerException` being thrown from within `addSuppressed`. I think it's 
better to just copy over (rest of) the finally block from 
`FilterOutputStream`'s close() method.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1439328181

Integrated: 8322027: One XMLStreamException constructor fails to initialize cause

[Archie Cobbs]

On Wed, 13 Dec 2023 20:06:03 GMT, Archie Cobbs  wrote:

> One of the three `XMLStreamException` constructors that takes a `Throwable` 
> fails to pass it to the superclass constructor.
> 
> This simple patch fixes that omission.
> 
> It's worth considering if there is any code out there that is working around 
> this problem already by invoking `initCause()` manually. If so, that code 
> would start throwing an `IllegalStateException` after this change.
> 
> So a more conservative fix would be to just add another constructor taking 
> the same arguments in a different order. But then again that's not much 
> better than just saying "always use initCause() with the broken constructor", 
> i.e., don't change anything.
> 
> Hmm.

This pull request has now been integrated.

Changeset: 5852f3ea
Author:Archie Cobbs 
Committer: Jaikiran Pai 
URL:   
https://git.openjdk.org/jdk/commit/5852f3eafe4509a064c727371962ff249886e115
Stats: 63 lines in 2 files changed: 62 ins; 0 del; 1 mod

8322027: One XMLStreamException constructor fails to initialize cause

Reviewed-by: joehw, jpai

-

PR: https://git.openjdk.org/jdk/pull/17090

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v4]

[Jaikiran Pai]

On Tue, 2 Jan 2024 09:31:16 GMT, Eirik Bjørsnøs  wrote:

>> Please review this test-only PR which adds test coverage for 
>> `ZipFile.getEntry` under certain charset conditions. 
>> 
>> When `ZipFile.getEntry` is called for an entry which has the `Language 
>> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
>> unconditionally, even when a different charset was supplied to the `ZipFile` 
>> constructor.
>> 
>> It turns out we do not have any testing for this particular case, as can be 
>> verified by commenting out the following line of code in 
>> `ZipFile.Source.getEntryPos`:
>> 
>> 
>> //ZipCoder zc = zipCoderForPos(pos);
>> ``` 
>> 
>> and then running `make test TEST="test/jdk/java/util/zip"`
>> 
>> The current test verifies that the correct ZipCoder is used by 
>> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
>> 
>> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
>> (accidentally?) fixed by 
>> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
>> worthwhile to add explicit testing for this case to avoid regressions.
>> 
>> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
>> JUnit 5. The conversion and modernization of the code is done in the first 
>> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
>> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
>> required to verify the `Language encoding flag` condition for 
>> `ZipFile.getEntry`.
>> 
>> Testing: Verified that the test indeed fails when 
>> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
>> suggested above.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add more cases for 'language encoding' bit set, opened with a different 
> encoding

Thank you for the update. This looks good to me.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17207#pullrequestreview-1800190473

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v3]

[Eirik Bjørsnøs]

On Tue, 2 Jan 2024 08:02:46 GMT, Jaikiran Pai  wrote:

> The change to allow user/application specific arbritary charsets to the 
> `ZipFile` constructor seems to have been done long back in Java 1.7 days as 
> part of https://bugs.openjdk.org/browse/JDK-4244499.
> 
> This is merely an observation and I don't expect any changes to the ZipFile 
> source or any of your proposed tests, in this context.

I agree that changing this long-standing behavior now is probably not worth the 
risk. Also, there are probably non-compliant ZIPs out there worth opening.

We could perhaps consider adding a note to the documentation of `ZipFile(File, 
Charset)` mentioning that the only fully standards-compliant charset is 'IBM 
Code Page 437'. However, I think that's out of scope for this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/17207#issuecomment-1873790654

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v3]

[Eirik Bjørsnøs]

On Tue, 2 Jan 2024 07:43:03 GMT, Jaikiran Pai  wrote:

>> Eirik Bjørsnøs has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Add @bug tag for 8322802
>
> test/jdk/java/util/zip/ZipCoding.java line 86:
> 
>> 84: // ZipOutputStream sets the 'Language encoding flag' 
>> when writing using UTF-8
>> 85: // UTF-8 should be used for decoding, even when opening 
>> with a different charset
>> 86: Arguments.of("utf-8", "iso-8859-1",
> 
> Hello Eirik, there are 3 streams of arguments above which use "utf-8" for 
> writing out the entry. All those other 3 streams of arguments, use "utf-8" 
> for reading too (which is OK). For each of those 3 streams of arguments, 
> could you also add another argument stream which uses non-utf8 charset for 
> reading, like you do here? I think that would give this a bit more coverage, 
> especially for cases like the surrogates in comments and entry names.

Thanks, these cases got lost during my parameterization. I've added them back 
now. 

One such case would probably suffice to verify that the bit flag was 
interpreted correctly, but it doesn't hurt to have some more variation here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17207#discussion_r1439274478

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v4]

[Eirik Bjørsnøs]

> Please review this test-only PR which adds test coverage for 
> `ZipFile.getEntry` under certain charset conditions. 
> 
> When `ZipFile.getEntry` is called for an entry which has the `Language 
> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
> unconditionally, even when a different charset was supplied to the `ZipFile` 
> constructor.
> 
> It turns out we do not have any testing for this particular case, as can be 
> verified by commenting out the following line of code in 
> `ZipFile.Source.getEntryPos`:
> 
> 
> //ZipCoder zc = zipCoderForPos(pos);
> ``` 
> 
> and then running `make test TEST="test/jdk/java/util/zip"`
> 
> The current test verifies that the correct ZipCoder is used by 
> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
> 
> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
> (accidentally?) fixed by 
> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
> worthwhile to add explicit testing for this case to avoid regressions.
> 
> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
> JUnit 5. The conversion and modernization of the code is done in the first 
> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
> required to verify the `Language encoding flag` condition for 
> `ZipFile.getEntry`.
> 
> Testing: Verified that the test indeed fails when 
> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
> suggested above.

Eirik Bjørsnøs has updated the pull request incrementally with one additional 
commit since the last revision:

  Add more cases for 'language encoding' bit set, opened with a different 
encoding

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17207/files
  - new: https://git.openjdk.org/jdk/pull/17207/files/310c12a3..4f53308c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17207=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=17207=02-03

  Stats: 13 lines in 1 file changed: 11 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/17207.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17207/head:pull/17207

PR: https://git.openjdk.org/jdk/pull/17207

Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v2]

[Eirik Bjørsnøs]

On Tue, 2 Jan 2024 08:35:47 GMT, Jaikiran Pai  wrote:

> This call has a potential to throw an `IOException` in which case any 
> original `IOException` thrown from the `finish()` call will be lost.

Good catch. Adopted your suggestion with a few changes:

- The catch for IOException during finish() needs to rethrow the exception
- The catch for IOException during close() needs to check that finishException 
!= ioe, otherwise addSuppressed may throw IAE, see 
https://bugs.openjdk.org/browse/JDK-8042377
- I renamed finishFailure to finishException

The above changes were inspired by the overridden method 
`FilterOutputStream.close()`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1439265718

Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream [v2]

[Eirik Bjørsnøs]

> Please consider this PR which makes `DeflaterOutputStream.close()` always 
> close its wrapped output stream.
> 
> Currently, closing of the wrapped output stream happens outside the finally 
> block where `finish()` is called. If `finish()` throws, this means the 
> wrapped stream will not be closed. This can potentially lead to leaking 
> resources such as file descriptors or sockets.
> 
> This fix is to move the closing of the wrapped stream inside the finally 
> block.
> 
> Specification: This change brings the implementation of 
> `DeflaterOutputStream.close()` in line with its specification:  *Writes 
> remaining compressed data to the output stream and closes the underlying 
> stream.*
> 
> Risk: This is a behavioural change. There is a small risk that existing code 
> depends on the close method not following its specification.
> 
> Testing: The PR adds a new JUnit 5 test `CloseWrappedStream.java` which 
> simulates the failure condition and verifies that the wrapped stream was 
> closed under failing and non-failing conditions.

Eirik Bjørsnøs has updated the pull request incrementally with one additional 
commit since the last revision:

  Prevent IOException thrown during finish() from being lost if an IOException 
is thrown while closing the wrapped stream

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17209/files
  - new: https://git.openjdk.org/jdk/pull/17209/files/af21bda7..02707d58

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17209=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=17209=00-01

  Stats: 12 lines in 1 file changed: 11 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/17209.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17209/head:pull/17209

PR: https://git.openjdk.org/jdk/pull/17209

Re: RFR: 8313739: ZipOutputStream.close() should always close the wrapped stream

[Jaikiran Pai]

On Mon, 1 Jan 2024 16:12:13 GMT, Eirik Bjørsnøs  wrote:

> Specification: This change brings the implementation of 
> DeflaterOutputStream.close() in line with its specification: Writes remaining 
> compressed data to the output stream and closes the underlying stream.

> Risk: This is a behavioural change. There is a small risk that existing code 
> depends on the close method not following its specification.

I think this won't require a CSR since the change is merely fixing an issue and 
making it comply with the specification.

src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 249:

> 247: def.end();
> 248: }
> 249: out.close();

This call has a potential to throw an `IOException` in which case any original 
`IOException` thrown from the `finish()` call will be lost.

Perhaps we should do something like:


public void close() throws IOException {
if (!closed) {
IOException finishFailure = null;
try {
finish();
} catch (IOException ioe){
finishFailure = ioe;
} finally {
if (usesDefaultDeflater) {
def.end();
}
try {
out.close();
} catch (IOException ioe) {
if (finishFailure != null) {
ioe.addSuppressed(finishFailure);
}
throw ioe;
}
closed = true;
}
}
}

-

PR Comment: https://git.openjdk.org/jdk/pull/17209#issuecomment-1873737464
PR Review Comment: https://git.openjdk.org/jdk/pull/17209#discussion_r1439238691

Re: RFR: 8322802: Add testing for ZipFile.getEntry respecting the 'Language encoding' flag [v3]

[Jaikiran Pai]

On Mon, 1 Jan 2024 14:29:50 GMT, Eirik Bjørsnøs  wrote:

>> Please review this test-only PR which adds test coverage for 
>> `ZipFile.getEntry` under certain charset conditions. 
>> 
>> When `ZipFile.getEntry` is called for an entry which has the `Language 
>> encoding flag` general purpose bit flag set,  then `ZipCoder.UTF8` is used 
>> unconditionally, even when a different charset was supplied to the `ZipFile` 
>> constructor.
>> 
>> It turns out we do not have any testing for this particular case, as can be 
>> verified by commenting out the following line of code in 
>> `ZipFile.Source.getEntryPos`:
>> 
>> 
>> //ZipCoder zc = zipCoderForPos(pos);
>> ``` 
>> 
>> and then running `make test TEST="test/jdk/java/util/zip"`
>> 
>> The current test verifies that the correct ZipCoder is used by 
>> `ZipFile.entries()`, but does not exercise `ZipFile.getEntry` the same way.
>> 
>> Seeing that [JDK-7009069](https://bugs.openjdk.org/browse/JDK-7009069) was 
>> (accidentally?) fixed by 
>> [JDK-8243469](https://bugs.openjdk.org/browse/JDK-8243469), I think it is 
>> worthwhile to add explicit testing for this case to avoid regressions.
>> 
>> While visiting `ZipCoding.java`, I took the opportunity to convert it to 
>> JUnit 5. The conversion and modernization of the code is done in the first 
>> commit 1384850ed51ec845af06dd6d13616f20f8bbaa6a in this PR, while the second 
>> commit 1776b258b0fe8383709ae0c095f2631a4e6237f6 actually adds the code 
>> required to verify the `Language encoding flag` condition for 
>> `ZipFile.getEntry`.
>> 
>> Testing: Verified that the test indeed fails when 
>> `ZipFile.Source.getEntryPos` is updated to use the ZipFile's ZipCoder as 
>> suggested above.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add @bug tag for 8322802

This is a test-only change PR and the changes being proposed look good to me.

On a general note - I hadn't paid attention to this ZipFile constructor which 
accepts a character encoding, before. I read up on the Zip file specific today 
(https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT) to understand it 
a bit more. Interestingly, in that spec (if I read it correctly), unless the 
`0x0008` "extended language encoding" header is set, there isn't anything that 
says/allows the entry name or entry comment to be anything other than either 
UTF-8 or "IBM Code Page 437" character encoding:


APPENDIX D - Language Encoding (EFS)


D.1 The ZIP format has historically supported only the original IBM PC 
character 
encoding set, commonly referred to as IBM Code Page 437.  This limits storing 
file name characters to only those within the original MS-DOS range of values 
and does not properly support file names in other character encodings, or 
languages. To address this limitation, this specification will support the 
following change. 

D.2 If general purpose bit 11 is unset, the file name and comment SHOULD 
conform 
to the original ZIP character encoding.  If general purpose bit 11 is set, the 
filename and comment MUST support The Unicode Standard, Version 4.1.0 or 
greater using the character encoding form defined by the UTF-8 storage 
specification.  


D.3 Applications MAY choose to supplement this file name storage through the 
use 
of the 0x0008 Extra Field.  Storage for this optional field is currently 
undefined, however it will be used to allow storing extended information 
on source or target encoding that MAY further assist applications with file 
name, or file content encoding tasks.  Please contact PKWARE with any
requirements on how this field SHOULD be used.

D.4 The 0x0008 Extra Field storage MAY be used with either setting for general 
purpose bit 11.  Examples of the intended usage for this field is to store 
whether "modified-UTF-8" (JAVA) is used, or UTF-8-MAC.  Similarly, other 
commonly used character encoding (code page) designations can be indicated 
through this field.  Formalized values for use of the 0x0008 record remain 
undefined at this time.  The definition for the layout of the 0x0008 field
will be published when available.  Use of the 0x0008 Extra Field provides
for storing data within a ZIP file in an encoding other than IBM Code
Page 437 or UTF-8.


The change to allow user/application specific arbritary charsets to the 
`ZipFile` constructor seems to have been done long back in Java 1.7 days as 
part of https://bugs.openjdk.org/browse/JDK-4244499.

This is merely an observation and I don't expect any changes to the ZipFile 
source or any of your proposed tests, in this context.

-

PR Comment: https://git.openjdk.org/jdk/pull/17207#issuecomment-1873711203