Re: [15] RFR: 8239520: ValueRange.of(long, long, long) does not throw IAE on invalid inputs

[naoto . sato]

Hi Roger,

Thank you for the review. Please find my comments below.

On 2/21/20 11:42 AM, Roger Riggs wrote:
In the implementation ValueRange.of: 148-150; I would omit the check 
here since

it invokes the 4 arg form and does the same check.
The difference in exception message is not worth the extra code.
Though it would help if it said that the 3 arg form is the same as the 4 
arg form with the same first two args.


I followed the pattern in the 2 arg form which adds a condition with its 
own exception message, and also invokes 4 arg form.




In the test TestDateTimeValueRange.java, the separate test case is 
(mostly) redundant with the added case at line 187.
(Though that assumes the 3 arg form calls the 4 arg form; as does the 
lack of any other testing of the 3 arg form).


Since this is a test, I would rather not rely on the implementation 
detail that 3 arg form calls into 4 arg form. Eventual redundancy in 
test would not be an issue, IMO.




Also,
It might be useful to verify that each of the test cases 179-186 trigger 
only 1 of the range checks.
If a test case would fail more than 1 of the conditions then it might be 
never be testing each if statements.


I specifically added variations that only verify each condition. 
Modified the test case:


http://cr.openjdk.java.net/~naoto/8239520/webrev.01/

Naoto



Thanks, Roger


On 2/20/20 5:28 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8239520

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8239520/webrev.00/

This change lets 3-arg ValueRange.of() method thrown an IAE on invalid 
inputs, as well as corrects the method description of 4-arg 
ValueRange.of() method which lacks an IAE condition. I also filed a 
CSR for this change accordingly:


https://bugs.openjdk.java.net/browse/JDK-8239574

Naoto

Re: ZoneRules.of() implies that transitionList is a superset of standardOffsetTransitionList but doesn't check that

[Stuart Marks]

Was any bug ever filed for this? I looked in JBS and I couldn't find anything 
that resembled the issue here.


Roman, since you don't have an account on JBS (bugs.openjdk.java.net), I'd 
suggest you file a bug via bugs.java.com. Bugs filed there do eventually get 
into JBS. (I believe it's difficult for submitters to track how this happens, 
but it does work.)


But earlier, you had said,


java.bugs.com you mean? Not long ago I reported a similar issue about core
libs and they recommended me to write to list directly, so that's why I
wrote directly here this time.


Who said that? That's strange. The preferred route for people who don't have JBS 
to submit bugs against the JDK is indeed bugs.java.com. Please contact us if it 
happens again.


Of course if you have questions or relevant issues to discuss, you're always 
welcome to come here or to other OpenJDK mailing lists.


s'marks



On 1/31/20 10:05 AM, Roman Leventov wrote:

I cannot create an issue because I'm not an OpenJDK process member.

Yes, the test constructs an invalid ZoneRules, the point is to demonstrate
that this is possible and IAE is not thrown upon construction. I don't
think this is a good principle for immutable classes. Once they are
constructed, their state must be verified to be correct.

On Fri, 31 Jan 2020 at 20:17,  wrote:


I meant the JDK Bug System:

https://bugs.openjdk.java.net/

All the issues in the JDK is tracked here.

As to the issues themselves, you would want to specify
Arrays.asList(trans) in place for the 4th argument on ZoneRules.of(), so
that the transition happens on the transition day?

For 2), good catch for the missing 'i', we need a JBS issue for that to
fix it.

Naoto

On 1/31/20 6:08 AM, Roman Leventov wrote:

java.bugs.com  you mean? Not long ago I reported

a

similar issue about core libs and they recommended me to write to list
directly, so that's why I wrote directly here this time.

On Fri, 31 Jan 2020 at 16:57, mailto:naoto.s...@oracle.com>> wrote:

 Hi Roman,

 Please file a JBS issue for this.

 Naoto

 On 1/29/20 9:56 PM, Roman Leventov wrote:
  > 1) ZoneRules.of() implies that transitionList is a superset of
  > standardOffsetTransitionList but doesn't check that. Then it's
 possible to
  > construct ZoneRules instances that don't work correctly:
  >  @Test
  >  public void zoneRulesTest() {
  >  LocalDateTime transitionDay = LocalDateTime.of(2020, 1,
 1, 2, 0);
  >  ZoneOffsetTransition trans = ZoneOffsetTransition.of(
  >  transitionDay,
  >  ZoneOffset.ofHours(1),
  >  ZoneOffset.ofHours(2)
  >  );
  >  ZoneRules rules = ZoneRules.of(ZoneOffset.ofHours(1),
  > ZoneOffset.ofHours(1),
  >  Arrays.asList(trans),
  >  Collections.emptyList(),

Collections.emptyList());

  >
  >  Assert.assertEquals(ZoneOffset.ofHours(2),

rules.getOffset(

  >
 transitionDay.plusDays(7).toInstant(ZoneOffset.UTC)));
  >  }
  >
  > 2) Unrelated issue in java-time code: in
 ZoneOffsetTransitionRule, there is
  > a typo, some variables are called "timeDefnition" (missed "i")
 and it leaks
  > to public Javadoc.
  >

Re: RFR: JDK-8238692: MacOS runtime Installer issue

[Alexander Matveev]

Hi Andy,

Looks good.

Thanks,
Alexander

On 2/21/2020 9:10 AM, Andy Herrick wrote:
After internal discussion of default values for mac-package-identifier 
I have removed the fix for [3] from this main fix to issue [2].


please review the revised webrev at [1]

/Andy

On 2/18/2020 2:46 PM, Andy Herrick wrote:


Please review the jpackage fix at [1] for the related issues [2], 
[3], and [4].


This will basically make pkg type runtime installers work on macosx, 
while we wait for JDK-8237971 to either fix or disable dmg type 
runtime installers.


/Andy

[1] http://cr.openjdk.java.net/~herrick/8238692/webrev.02/

[2] https://bugs.openjdk.java.net/browse/JDK-8238692

[3] https://bugs.openjdk.java.net/browse/JDK-8237966

[4] https://bugs.openjdk.java.net/browse/JDK-8237970

Re: RFR [15] 8161558: ListIterator should not discard cause on exception

[Stuart Marks]

On 2/14/20 2:34 PM, fo...@univ-mlv.fr wrote:

The thing is that inside the iterator, you already have the right information, 
so you don't have to pray to have the right info.


No, the iterator only can guess at the reason that get() threw the exception.


I just disagree on the conclusion, chaining exception is great when you bubble 
up an information that make sense to the end-user,
knowing that next() is implemented using get() that why you get an IOOBE as 
cause of the NSEE is just noise for a end-user.


If the IOOBE is noise, it can be ignored. But suppose the programmer is 
implementing a list by subclassing AbstractList, and they have some internal 
inconsistency that causes their code to throw IOOBE unexpectedly. AbstractList 
does this programmer a disservice by THROWING AWAY the nice stack trace and 
message generated by the subclass or possibly by the VM.


Recall that as an abstract class, AbstractList has end users (callers) as well 
as subclassers, and it needs to provide services to both. Chaining the 
exceptions is a service to subclass implementors.


s'marks

Re: [15] RFR: 8239520: ValueRange.of(long, long, long) does not throw IAE on invalid inputs

[Roger Riggs]

Hi Naoto,

The CSR looks ok.

In the implementation ValueRange.of: 148-150; I would omit the check 
here since

it invokes the 4 arg form and does the same check.
The difference in exception message is not worth the extra code.
Though it would help if it said that the 3 arg form is the same as the 4 
arg form with the same first two args.


In the test TestDateTimeValueRange.java, the separate test case is 
(mostly) redundant with the added case at line 187.
(Though that assumes the 3 arg form calls the 4 arg form; as does the 
lack of any other testing of the 3 arg form).


Also,
It might be useful to verify that each of the test cases 179-186 trigger 
only 1 of the range checks.
If a test case would fail more than 1 of the conditions then it might be 
never be testing each if statements.


Thanks, Roger


On 2/20/20 5:28 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8239520

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8239520/webrev.00/

This change lets 3-arg ValueRange.of() method thrown an IAE on invalid 
inputs, as well as corrects the method description of 4-arg 
ValueRange.of() method which lacks an IAE condition. I also filed a 
CSR for this change accordingly:


https://bugs.openjdk.java.net/browse/JDK-8239574

Naoto

RE: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in zipfs implementation

[Langer, Christoph]

Hi Lance,

thanks, I’ve pushed it now: http://hg.openjdk.java.net/jdk/jdk/rev/c22b369d40b2

I’ll be out next week, so I’ll probably get to JDK-8225507 the week after.

Cheers
Christoph

From: Lance Andersen 
Sent: Freitag, 21. Februar 2020 12:34
To: Langer, Christoph 
Cc: nio-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Subject: Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in 
zipfs implementation

Morning Christoph,



On Feb 21, 2020, at 3:11 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi Lance,

thanks for reviewing. Any results from Mach5? I’ll push it after I get your ok.

It finished late last night and it ran clean


The manifest fix will still apply after this – but you wanted to update the 
webrev anyway with some test updates, right?
Yes, I wanted to clean up the test some.


When both of these fixes are in, I’ll look at augmenting Jai’s current proposal 
for JDK-8225507 to overhaul exception handling in close()().

Assume your last webrev on top of this fix is the lucky winner while you take a 
stab at this clean up :-)

Best
Lance


Cheers
Christoph



From: Lance Andersen 
mailto:lance.ander...@oracle.com>>
Sent: Donnerstag, 20. Februar 2020 20:00
To: Langer, Christoph 
mailto:christoph.lan...@sap.com>>
Cc: nio-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Subject: Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in 
zipfs implementation

Hi Christoph,

This looks good and thank you for tackling. We will need to regenerate the 
web-rev for the Manifest fix after this gets pushed

I am running Mach5 tiers 1-3 now and will let you know when it completes





On Feb 20, 2020, at 8:33 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi,

please review this cleanup change to remove the ExistingChannelCloser facility 
in zipfs.

Some technical details about why this existed can be found in this mailing list 
thread, where Sherman explained its original purpose: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059839.html. 
At that time we didn't dare to remove it and deferred to further investigation.

Now, when looking at the close() implementation of ZipFileSystem during review 
of JDK-8225507, I had a closer look to exChannelCloser as well and found that 
it should definitely be removed. Its original purpose was to keep a backing 
file and channel for InputStreams that were still open while the sync() method 
was called to update a zip file on disk. However, in its current state, a zip 
file is only synced to disk by sync() during close(), at a time when all 
InputStreams should already be closed.

Closing of the streams is done at the beginning of method close(), in line 
466ff. Closing of any open InputStream would remove it from the list named 
"streams". After that, it must be assured that no InputStreams get created, of 
course. This is accomplished by calling ensureOpen() under readLock, whenever 
an InputStream shall be created. The first step of close() though is to set the 
ZipFileSystem to state "closed", so these checks should all fail. I have, 
however, found one location where this ensureOpen check was missing and added 
it.

jdk/nio/zipfs tests still pass.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8239556.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8239556

Thanks
Christoph



Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com






[cid:image001.gif@01D5E8E4.A340D420]

Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: jpackage current status

[Michael Hall]

> On Feb 21, 2020, at 11:12 AM, Kevin Rushforth  
> wrote:
> 
> I doubt this has anything to do with jpackage being in incubator or not. 
> Fundamentally, just copying binary launchers into another JDK image like you 
> are doing is only going to work by accident, if it works at all. If you need 
> jpackage (or javac or jar or ...) to be in a JDK image, then you should jlink 
> it yourself, include all of the modules you need, and don't strip the 
> executables.
> 
> -- Kevin
> 

I was doing that for applications going back to before jlink - application 
build tools have not handled this real consistently and often just strip. 
Possibly now you are correct. jpackage does use jlink though? So why would my 
own be better than what the tool does? I believe I showed the module is present 
in the runtime in use. 
But again doing jlink separately might in this instance be the best practice. I 
will try it.

Re: RFR: JDK-8238692: MacOS runtime Installer issue

[Andy Herrick]

After internal discussion of default values for mac-package-identifier I 
have removed the fix for [3] from this main fix to issue [2].


please review the revised webrev at [1]

/Andy

On 2/18/2020 2:46 PM, Andy Herrick wrote:


Please review the jpackage fix at [1] for the related issues [2], [3], 
and [4].


This will basically make pkg type runtime installers work on macosx, 
while we wait for JDK-8237971 to either fix or disable dmg type 
runtime installers.


/Andy

[1] http://cr.openjdk.java.net/~herrick/8238692/webrev.02/

[2] https://bugs.openjdk.java.net/browse/JDK-8238692

[3] https://bugs.openjdk.java.net/browse/JDK-8237966

[4] https://bugs.openjdk.java.net/browse/JDK-8237970

Re: jpackage current status

[Kevin Rushforth]

I doubt this has anything to do with jpackage being in incubator or not. 
Fundamentally, just copying binary launchers into another JDK image like 
you are doing is only going to work by accident, if it works at all. If 
you need jpackage (or javac or jar or ...) to be in a JDK image, then 
you should jlink it yourself, include all of the modules you need, and 
don't strip the executables.


-- Kevin


On 2/21/2020 9:04 AM, Michael Hall wrote:



On Feb 21, 2020, at 9:39 AM, Michael Hall  wrote:


You can't copy launchers in this way as it requires the module to be in the 
run-time image.

If I add modules it into the build runtime I think I’m ok but haven’t tried it 
yet.

jpackage seems to need more than just the module in the run-time image.
I did the jpackage build again including it in the --add-modules

This now shows it there…

exec java --list-modules | grep jpackage
System.in:35:jdk.incubator.jpackage@14

However, I still get this…

exec jpackage --version
java.io.IOException: Cannot run program "jpackage": error=2, No such file or 
directory
at java.base/java.lang.ProcessBuilder.start(Unknown Source)
at java.base/java.lang.ProcessBuilder.start(Unknown Source)

Running it directly now works…

./jpackage --version
WARNING: Using incubator modules: jdk.incubator.jpackage
14

Which I think again shows the module is included in the run-time image.

Possibly there is some other dependency. I’ll have to figure that out or wait 
until jpackage is out of incubator.

Re: jpackage current status

[Michael Hall]

> On Feb 21, 2020, at 9:39 AM, Michael Hall  wrote:
> 
>> You can't copy launchers in this way as it requires the module to be in the 
>> run-time image.
> 
> If I add modules it into the build runtime I think I’m ok but haven’t tried 
> it yet.

jpackage seems to need more than just the module in the run-time image. 
I did the jpackage build again including it in the --add-modules

This now shows it there…

exec java --list-modules | grep jpackage
System.in:35:jdk.incubator.jpackage@14

However, I still get this…

exec jpackage --version
java.io.IOException: Cannot run program "jpackage": error=2, No such file or 
directory
at java.base/java.lang.ProcessBuilder.start(Unknown Source)
at java.base/java.lang.ProcessBuilder.start(Unknown Source)

Running it directly now works…

./jpackage --version
WARNING: Using incubator modules: jdk.incubator.jpackage
14

Which I think again shows the module is included in the run-time image.

Possibly there is some other dependency. I’ll have to figure that out or wait 
until jpackage is out of incubator.

Re: RFR: 8235834 IBM-943 charset encoder needs updating

[naoto . sato]

Two subtle comments to the new webrev:

- I'd add "private" to those static finals.
- "cs.name()" in the exception messages can be replaced with "csName".

Otherwise it looks good. If you agree with the above, no further review 
is needed. Also, if you need a sponsor, I can sponsor your changeset.


Naoto

On 2/21/20 5:10 AM, Ichiroh Takiguchi wrote:

Hello Naoto.

I appreciate your suggestions.
I applied your suggestions into new patch.
Could you review the fix again ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8235834
Change: https://cr.openjdk.java.net/~itakiguchi/8235834/webrev.01/

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2020-02-21 02:51, naoto.s...@oracle.com wrote:

Thanks for the explanation. Here are comments to your fix:

- Can you add some comments about the gist of the change to
IBM943.c2b? Summarizing the explanation below would be fine.

For the test case:

- Please wrap the @bug line appropriately.

- Those byte array/Strings can be "static final" outside the test method.

- Looks like the mapping is exactly the same with IBM943C for those
code points. Is that correct? If so, would you please add some comment
as such?

- Typo: "round-tip" -> "round-trip"

Naoto

On 2/20/20 2:47 AM, Ichiroh Takiguchi wrote:

Hello Naoto.

I appreciate your comment.

The definition has not changed recently.
Applying the change will prevent the characters which are used on 
Japanese PC from being converted to "?".


I checked IBM-943 definition and changelog.

Definitions and creation date are as follows:
(All definitions are valid)
03AF34B0.TPMAP120: May 20 1997 （Current OpenJDK)
34B003AF.RPMAP130: May 20 1997 (Proposed change, upward compatible)
34B003AF.RPMAP14A: Jul 29 1998 (Same as 34B003AF.RPMAP130)
34B003AF.RPMAP15A: Jan  8 2003 (Additionally, 13 characters are changed)

According to IBM943.map, OpenJDK JDK refers 03AF34B0.TPMAP120.
03AF34B0.TPMAP120 just has B2C conversion table only, no C2B definition.
34B003AF.RPMAP130 which has C2B definition was released on same date.
I assume C2B definition was not implemented at that time.

C2B definition for 34B003AF.RPMAP130 and 34B003AF.RPMAP14A are same, 
only replacement character is changed.

34B003AF.RPMAP15A is the latest, but it's almost same as MS932.
If 34B003AF.RPMAP15A is applied, 03AF34B0.TPMAP14A is also required.
I'd like to add C2B definition without B2C definition because of 
compatibility.

I don't want to apply 03AF34B0.TPMAP14A B2C definition.

So I'd like to apply 34B003AF.RPMAP130 definition.

Thanks,
Ichiroh Takiguchi

On 2020-02-19 07:33, naoto.s...@oracle.com wrote:

Hi Takiguchi-san,

Can you please elaborate the rationale for the change? It looks like
IBM943 chaset hasn't changed for a long time, at least from JDK8. Has
the mapping definition recently changed?

Naoto

On 2/17/20 10:23 AM, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8235834
Change: https://cr.openjdk.java.net/~itakiguchi/8235834/webrev.00/

IBM-943 is for IBM Japanese PC encoding.
MS932 is for Microsoft Japanese Windows encoding.
IBM-943 charset encoder does not contain 5 compatible entries 
compared to MS932 charset.


Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

Re: jpackage current status

[Michael Hall]

> On Feb 21, 2020, at 9:41 AM, Michael Hall  wrote:
> 
> 
> 
>> On Feb 21, 2020, at 9:39 AM, Philip Race  wrote:
>> 
>> Hi,
>> 
>> I don't understand what you are doing here.
>> It is like you are copying around an application
>> which has dependent libraries but not copying those libraries.
>> 
>> Copying bits out of the JDK to some other location isn't something you can 
>> expect to work.
> 
> It has worked for me in the past for the java command when applications need 
> that and the build has not included it.
> 
>> 
>> And why does your application need jpackage ?
>> It is a developer tool, not a run time tool ?
>> 
> 
> It runs commands. I am considering a jpackage wrapper command. Possibly 
> including some use of jlink.
> 

For example…

exec java -version
openjdk version "14" 2020-03-17
OpenJDK Runtime Environment (build 14+36-1461)
OpenJDK 64-Bit Server VM (build 14+36-1461, mixed mode, sharing)

exec jshell -version
jshell 14


However,

exec jpackage --help
java.io.IOException: Cannot run program "jpackage": error=2, No such file or 
directory
at java.base/java.lang.ProcessBuilder.start(Unknown Source)
at java.base/java.lang.ProcessBuilder.start(Unknown Source)
at java.base/java.lang.Runtime.exec(Unknown Source)
...

Re: jpackage current status

[Michael Hall]

> On Feb 21, 2020, at 9:39 AM, Philip Race  wrote:
> 
> Hi,
> 
> I don't understand what you are doing here.
> It is like you are copying around an application
> which has dependent libraries but not copying those libraries.
> 
> Copying bits out of the JDK to some other location isn't something you can 
> expect to work.

It has worked for me in the past for the java command when applications need 
that and the build has not included it.

> 
> And why does your application need jpackage ?
> It is a developer tool, not a run time tool ?
> 

It runs commands. I am considering a jpackage wrapper command. Possibly 
including some use of jlink.

Re: jpackage current status

[Michael Hall]

> On Feb 21, 2020, at 9:36 AM, Alan Bateman  wrote:
> 
> 
> 
> On 21/02/2020 15:23, Michael Hall wrote:
>> If I look at the jpackage page…
>> 
>> The jpackage tool has been integrated into the JDK, and is now included in 
>> JDK 14 early access builds.
>> 
>> The normal early access page now indicates release candidate and the 
>> download no longer includes -ea
>> 
>> However, the included jpackage still shows…
>> 
>>  jpackage --help
>> WARNING: Using incubator modules: jdk.incubator.jpackage
>> ...
>> 
>> With an incubator warning.
> Yes, should be expected as its an incubating module in JDK 14.  The CLI 
> options and application layout are non-final. The "Deliverying jpackage" 
> section in JEP 343 has more this.

I’ll look at the JEP as well.

> 
>> 
>> If I copy the jpackage bin command into my application
>> 
>> ./jpackage --help
>> Error occurred during initialization of boot layer
>> java.lang.module.FindException: Module jdk.incubator.jpackage not found
>> 
>> I could add the indicated jdk.incubator.jpackage module but should it 
>> actually still be needed?
> You can't copy launchers in this way as it requires the module to be in the 
> run-time image.

If I add modules it into the build runtime I think I’m ok but haven’t tried it 
yet.

Re: jpackage current status

[Philip Race]

Hi,

I don't understand what you are doing here.
It is like you are copying around an application
which has dependent libraries but not copying those libraries.

Copying bits out of the JDK to some other location isn't something you 
can expect to work.


And why does your application need jpackage ?
It is a developer tool, not a run time tool ?

-phil.

On 2/21/20, 7:23 AM, Michael Hall wrote:

If I look at the jpackage page…

The jpackage tool has been integrated into the JDK, and is now included in JDK 
14 early access builds.

The normal early access page now indicates release candidate and the download 
no longer includes -ea

However, the included jpackage still shows…

  jpackage --help
WARNING: Using incubator modules: jdk.incubator.jpackage
...

With an incubator warning.

If I copy the jpackage bin command into my application

./jpackage --help
Error occurred during initialization of boot layer
java.lang.module.FindException: Module jdk.incubator.jpackage not found

I could add the indicated jdk.incubator.jpackage module but should it actually 
still be needed?

Re: jpackage current status

[Alan Bateman]

On 21/02/2020 15:23, Michael Hall wrote:

If I look at the jpackage page…

The jpackage tool has been integrated into the JDK, and is now included in JDK 
14 early access builds.

The normal early access page now indicates release candidate and the download 
no longer includes -ea

However, the included jpackage still shows…

  jpackage --help
WARNING: Using incubator modules: jdk.incubator.jpackage
...

With an incubator warning.
Yes, should be expected as its an incubating module in JDK 14.  The CLI 
options and application layout are non-final. The "Deliverying jpackage" 
section in JEP 343 has more this.




If I copy the jpackage bin command into my application

./jpackage --help
Error occurred during initialization of boot layer
java.lang.module.FindException: Module jdk.incubator.jpackage not found

I could add the indicated jdk.incubator.jpackage module but should it actually 
still be needed?
You can't copy launchers in this way as it requires the module to be in 
the run-time image.


-Alan

Re: RFR: 8239365: ProcessBuilder/Basic.java test has trouble on AIX sometimes

[Roger Riggs]

Hi Adam,

Is it possible for the test to control the message-set?
If so, it might be safer to be explicit about the message-set to be used.
(Similar to setting LC_CTYPE=C)

Adding the "error=nn" should be sufficient without matching the full text.
It will be more resilient to possible (though unlikely) changes in the 
messages.


The distinction between the "No such file" and "No such file or 
directory" is not significant and must have crept in with multiple 
authors.  Unix does not have two different messages for error=2 and the 
shorter one covers both.


I would collapse the cases to:

static final String PERMISSION_DENIED_ERROR_MSG = "(Permission 
denied|error=13)";

static final String NO_SUCH_FILE_ERROR_MSG = "(No such file|error=2)";

Regards, Roger

On 2/21/20 9:51 AM, Adam Farley8 wrote:

Hi All,

Reviews and sponsor requested for a small test change.

Parts of the test appear to be pattern-matching on error messages from
catalogues on the system.

When an AIX machine has the file set "bos.msg.en_US.rte", the messages are
different for the same codes.

E.g. for Error code 13 (in the relevant group, see libc.cat) we get:

*With* file set: error=13, The file access permissions do not allow the
specified action
*Without* file set: error=13, Permission denied

Since the pattern the test matches on is "Permission denied", it fails if
the aforementioned file set is installed.

The simplest option appears to be adding the second potential form of the
message into the regex (see webrev).

http://cr.openjdk.java.net/~afarley/8239365.1/webrev/

Another option is to have "error=x," replace the regex for each error
message, unless the format of this changes on an OS I haven't checked.

Bug: https://bugs.openjdk.java.net/browse/JDK-8239365

Best Regards

Adam Farley
IBM Runtimes

P.S. The bug database website appears to be busy restarting at the moment,
if you have difficulty accessing the second link.
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: jpackage current status

[Michael Hall]

> On Feb 21, 2020, at 9:27 AM, Scott Palmer  wrote:
> 
> It's included as a preview feature, so it's still incubating.
> 

OK, I’ll include the module. Remove it later.

Thanks

Re: jpackage current status

[Scott Palmer]

It's included as a preview feature, so it's still incubating.

On Fri, Feb 21, 2020 at 10:24 AM Michael Hall  wrote:

> If I look at the jpackage page…
>
> The jpackage tool has been integrated into the JDK, and is now included in
> JDK 14 early access builds.
>
> The normal early access page now indicates release candidate and the
> download no longer includes -ea
>
> However, the included jpackage still shows…
>
>  jpackage --help
> WARNING: Using incubator modules: jdk.incubator.jpackage
> ...
>
> With an incubator warning.
>
> If I copy the jpackage bin command into my application
>
> ./jpackage --help
> Error occurred during initialization of boot layer
> java.lang.module.FindException: Module jdk.incubator.jpackage not found
>
> I could add the indicated jdk.incubator.jpackage module but should it
> actually still be needed?

jpackage current status

[Michael Hall]

If I look at the jpackage page…

The jpackage tool has been integrated into the JDK, and is now included in JDK 
14 early access builds.

The normal early access page now indicates release candidate and the download 
no longer includes -ea

However, the included jpackage still shows…

 jpackage --help
WARNING: Using incubator modules: jdk.incubator.jpackage
...

With an incubator warning. 

If I copy the jpackage bin command into my application 

./jpackage --help
Error occurred during initialization of boot layer
java.lang.module.FindException: Module jdk.incubator.jpackage not found

I could add the indicated jdk.incubator.jpackage module but should it actually 
still be needed?

Re: RFR: 8239559: Cgroups v2: Incorrect detection logic on some systems

[Bob Vandette]

> On Feb 21, 2020, at 9:30 AM, Severin Gehwolf  wrote:
> 
> Hi Bob,
> 
> On Fri, 2020-02-21 at 09:11 -0500, Bob Vandette wrote:
>> Severin,
>> 
>> Don’t we need the contents of /proc/self/mountinfo in order to construct the 
>> path to the cgroup controllers?
> 
> There is only one for unified (cgroups v2), but yes it's beeing used.
> See CgroupV2Subsystem.initSubsystem() and
> CgroupV1Subsystem.initSubsystem(). For affected systems, no controllers
> are mounted, so the effect will be null Metrics, as before JDK-823. 
> Maybe I didn't understand the question, sorry.

If you don’t have access to the information required to get metrics, I just 
assumed that
you would return NULL in CgroupSubsystemFactory.create() rather than making the
assumption that it works only to fail later.

Alternatively, we could consider assuming the mount point is /sys/fs/cgroup for 
cgroupv1 in
the case you are trying to support.  This would involve using /proc/self/cgroup 
to get the list
of controllers and then use that list to call createSubSystemController and
setSubSystemControllerPath with the default path.

I think we need to understand the extent of the problem on these older systems 
before 
deciding a course of action.  Do we see the same empty mountinfo file in a 
docker container
running on these older systems or is this just a host issue?  If docker 
containers work fine, then
I wouldn’t bother trying to make this work.

Bob.

> 
>> On Thu, 2020-02-20 at 14:50 +, Baesken, Matthias wrote:
>>> Hi  Severin,
>>> 
>>> grep cgroup /proc/self/mountinfo 
>>> 
>>> returns  nothing.
>>> 
>>> Best Regards, Matthias
>>> 
>> 
>> Assuming your fix is correct, don’t we also need to apply the same change to 
>> the hotspot source cgroupSubsystem_linux.cpp?
> 
> Yes, tracked with JDK-8239785.
> 
> Thanks,
> Severin
> 
>> Bob;
>> 
>> 
>>> On Feb 21, 2020, at 8:32 AM, Severin Gehwolf  wrote:
>>> 
>>> Hi,
>>> 
>>> Could I please get a review of this fix to the detection heuristic of
>>> cgroup v1 vs cgroup v2? Matthias (in CC) discovered that on some old
>>> systems the JDK Metrics code throws InternalError caused by wrong
>>> detection logic when Metrics are being created on Linux.
>>> 
>>> The reason for this is that hierarchy IDs of 0 in /proc/cgroups is
>>> being used as a heuristic to detect cgroups v2 systems. Apparently some
>>> old systems like RHEL 6 and SLES 11 have no cgroups controllers
>>> mounted, thus, triggering a false positive.
>>> 
>>> The fix is to also look at /proc/self/mountinfo and correct logic in
>>> this case.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8239559
>>> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8239559/01/webrev/
>>> 
>>> Testing: docker/cgroups tests on hybrid (cgroups v1) and unified
>>> hierarchy (cgroups v2). New regression test. Looks good here.
>>> 
>>> Unfortunately, I wasn't able to reproduce this on an actual affected
>>> system. I somewhat reproduced via the derived regression test based on
>>> data from reporters. I'd appreciate any testing on systems where this
>>> reproduces.
>>> 
>>> Thanks,
>>> Severin
>>> 
> 

RFR: 8239365: ProcessBuilder/Basic.java test has trouble on AIX sometimes

[Adam Farley8]

Hi All,

Reviews and sponsor requested for a small test change.

Parts of the test appear to be pattern-matching on error messages from 
catalogues on the system.

When an AIX machine has the file set "bos.msg.en_US.rte", the messages are 
different for the same codes.

E.g. for Error code 13 (in the relevant group, see libc.cat) we get:

*With* file set: error=13, The file access permissions do not allow the 
specified action
*Without* file set: error=13, Permission denied

Since the pattern the test matches on is "Permission denied", it fails if 
the aforementioned file set is installed.

The simplest option appears to be adding the second potential form of the 
message into the regex (see webrev).

http://cr.openjdk.java.net/~afarley/8239365.1/webrev/

Another option is to have "error=x," replace the regex for each error 
message, unless the format of this changes on an OS I haven't checked.

Bug: https://bugs.openjdk.java.net/browse/JDK-8239365 

Best Regards

Adam Farley 
IBM Runtimes

P.S. The bug database website appears to be busy restarting at the moment, 
if you have difficulty accessing the second link.
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFR: 8239559: Cgroups v2: Incorrect detection logic on some systems

[Severin Gehwolf]

Hi Bob,

On Fri, 2020-02-21 at 09:11 -0500, Bob Vandette wrote:
> Severin,
> 
> Don’t we need the contents of /proc/self/mountinfo in order to construct the 
> path to the cgroup controllers?

There is only one for unified (cgroups v2), but yes it's beeing used.
See CgroupV2Subsystem.initSubsystem() and
CgroupV1Subsystem.initSubsystem(). For affected systems, no controllers
are mounted, so the effect will be null Metrics, as before JDK-823. 
Maybe I didn't understand the question, sorry.

> On Thu, 2020-02-20 at 14:50 +, Baesken, Matthias wrote:
> > Hi  Severin,
> > 
> > grep cgroup /proc/self/mountinfo 
> > 
> > returns  nothing.
> > 
> > Best Regards, Matthias
> > 
> 
> Assuming your fix is correct, don’t we also need to apply the same change to 
> the hotspot source cgroupSubsystem_linux.cpp?

Yes, tracked with JDK-8239785.

Thanks,
Severin

> Bob;
> 
> 
> > On Feb 21, 2020, at 8:32 AM, Severin Gehwolf  wrote:
> > 
> > Hi,
> > 
> > Could I please get a review of this fix to the detection heuristic of
> > cgroup v1 vs cgroup v2? Matthias (in CC) discovered that on some old
> > systems the JDK Metrics code throws InternalError caused by wrong
> > detection logic when Metrics are being created on Linux.
> > 
> > The reason for this is that hierarchy IDs of 0 in /proc/cgroups is
> > being used as a heuristic to detect cgroups v2 systems. Apparently some
> > old systems like RHEL 6 and SLES 11 have no cgroups controllers
> > mounted, thus, triggering a false positive.
> > 
> > The fix is to also look at /proc/self/mountinfo and correct logic in
> > this case.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8239559
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8239559/01/webrev/
> > 
> > Testing: docker/cgroups tests on hybrid (cgroups v1) and unified
> > hierarchy (cgroups v2). New regression test. Looks good here.
> > 
> > Unfortunately, I wasn't able to reproduce this on an actual affected
> > system. I somewhat reproduced via the derived regression test based on
> > data from reporters. I'd appreciate any testing on systems where this
> > reproduces.
> > 
> > Thanks,
> > Severin
> > 

Re: RFR: 8239559: Cgroups v2: Incorrect detection logic on some systems

[Bob Vandette]

Severin,

Don’t we need the contents of /proc/self/mountinfo in order to construct the 
path to the cgroup controllers?

On Thu, 2020-02-20 at 14:50 +, Baesken, Matthias wrote:
> Hi  Severin,
> 
> grep cgroup /proc/self/mountinfo 
> 
> returns  nothing.
> 
> Best Regards, Matthias
> 


Assuming your fix is correct, don’t we also need to apply the same change to 
the hotspot source cgroupSubsystem_linux.cpp?

Bob;


> On Feb 21, 2020, at 8:32 AM, Severin Gehwolf  wrote:
> 
> Hi,
> 
> Could I please get a review of this fix to the detection heuristic of
> cgroup v1 vs cgroup v2? Matthias (in CC) discovered that on some old
> systems the JDK Metrics code throws InternalError caused by wrong
> detection logic when Metrics are being created on Linux.
> 
> The reason for this is that hierarchy IDs of 0 in /proc/cgroups is
> being used as a heuristic to detect cgroups v2 systems. Apparently some
> old systems like RHEL 6 and SLES 11 have no cgroups controllers
> mounted, thus, triggering a false positive.
> 
> The fix is to also look at /proc/self/mountinfo and correct logic in
> this case.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8239559
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8239559/01/webrev/
> 
> Testing: docker/cgroups tests on hybrid (cgroups v1) and unified
> hierarchy (cgroups v2). New regression test. Looks good here.
> 
> Unfortunately, I wasn't able to reproduce this on an actual affected
> system. I somewhat reproduced via the derived regression test based on
> data from reporters. I'd appreciate any testing on systems where this
> reproduces.
> 
> Thanks,
> Severin
> 

RFR: 8239559: Cgroups v2: Incorrect detection logic on some systems

[Severin Gehwolf]

Hi,

Could I please get a review of this fix to the detection heuristic of
cgroup v1 vs cgroup v2? Matthias (in CC) discovered that on some old
systems the JDK Metrics code throws InternalError caused by wrong
detection logic when Metrics are being created on Linux.

The reason for this is that hierarchy IDs of 0 in /proc/cgroups is
being used as a heuristic to detect cgroups v2 systems. Apparently some
old systems like RHEL 6 and SLES 11 have no cgroups controllers
mounted, thus, triggering a false positive.

The fix is to also look at /proc/self/mountinfo and correct logic in
this case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8239559
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8239559/01/webrev/

Testing: docker/cgroups tests on hybrid (cgroups v1) and unified
hierarchy (cgroups v2). New regression test. Looks good here.

Unfortunately, I wasn't able to reproduce this on an actual affected
system. I somewhat reproduced via the derived regression test based on
data from reporters. I'd appreciate any testing on systems where this
reproduces.

Thanks,
Severin

Re: RFR: 8235834 IBM-943 charset encoder needs updating

[Ichiroh Takiguchi]

Hello Naoto.

I appreciate your suggestions.
I applied your suggestions into new patch.
Could you review the fix again ?

Bug:https://bugs.openjdk.java.net/browse/JDK-8235834
Change: https://cr.openjdk.java.net/~itakiguchi/8235834/webrev.01/

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2020-02-21 02:51, naoto.s...@oracle.com wrote:

Thanks for the explanation. Here are comments to your fix:

- Can you add some comments about the gist of the change to
IBM943.c2b? Summarizing the explanation below would be fine.

For the test case:

- Please wrap the @bug line appropriately.

- Those byte array/Strings can be "static final" outside the test 
method.


- Looks like the mapping is exactly the same with IBM943C for those
code points. Is that correct? If so, would you please add some comment
as such?

- Typo: "round-tip" -> "round-trip"

Naoto

On 2/20/20 2:47 AM, Ichiroh Takiguchi wrote:

Hello Naoto.

I appreciate your comment.

The definition has not changed recently.
Applying the change will prevent the characters which are used on 
Japanese PC from being converted to "?".


I checked IBM-943 definition and changelog.

Definitions and creation date are as follows:
(All definitions are valid)
03AF34B0.TPMAP120: May 20 1997 （Current OpenJDK)
34B003AF.RPMAP130: May 20 1997 (Proposed change, upward compatible)
34B003AF.RPMAP14A: Jul 29 1998 (Same as 34B003AF.RPMAP130)
34B003AF.RPMAP15A: Jan  8 2003 (Additionally, 13 characters are 
changed)


According to IBM943.map, OpenJDK JDK refers 03AF34B0.TPMAP120.
03AF34B0.TPMAP120 just has B2C conversion table only, no C2B 
definition.

34B003AF.RPMAP130 which has C2B definition was released on same date.
I assume C2B definition was not implemented at that time.

C2B definition for 34B003AF.RPMAP130 and 34B003AF.RPMAP14A are same, 
only replacement character is changed.

34B003AF.RPMAP15A is the latest, but it's almost same as MS932.
If 34B003AF.RPMAP15A is applied, 03AF34B0.TPMAP14A is also required.
I'd like to add C2B definition without B2C definition because of 
compatibility.

I don't want to apply 03AF34B0.TPMAP14A B2C definition.

So I'd like to apply 34B003AF.RPMAP130 definition.

Thanks,
Ichiroh Takiguchi

On 2020-02-19 07:33, naoto.s...@oracle.com wrote:

Hi Takiguchi-san,

Can you please elaborate the rationale for the change? It looks like
IBM943 chaset hasn't changed for a long time, at least from JDK8. Has
the mapping definition recently changed?

Naoto

On 2/17/20 10:23 AM, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8235834
Change: https://cr.openjdk.java.net/~itakiguchi/8235834/webrev.00/

IBM-943 is for IBM Japanese PC encoding.
MS932 is for Microsoft Japanese Windows encoding.
IBM-943 charset encoder does not contain 5 compatible entries 
compared to MS932 charset.


Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in zipfs implementation

[Lance Andersen]

Morning Christoph,


> On Feb 21, 2020, at 3:11 AM, Langer, Christoph  
> wrote:
> 
> Hi Lance,
>  
> thanks for reviewing. Any results from Mach5? I’ll push it after I get your 
> ok.

It finished late last night and it ran clean
>  
> The manifest fix will still apply after this – but you wanted to update the 
> webrev anyway with some test updates, right?
Yes, I wanted to clean up the test some.
>  
> When both of these fixes are in, I’ll look at augmenting Jai’s current 
> proposal for JDK-8225507 to overhaul exception handling in close()().

Assume your last webrev on top of this fix is the lucky winner while you take a 
stab at this clean up :-)

Best
Lance
>  
> Cheers
> Christoph
>  
>  
>  
> From: Lance Andersen  > 
> Sent: Donnerstag, 20. Februar 2020 20:00
> To: Langer, Christoph  >
> Cc: nio-...@openjdk.java.net ; 
> core-libs-dev@openjdk.java.net 
> Subject: Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in 
> zipfs implementation
>  
> Hi Christoph,
>  
> This looks good and thank you for tackling. We will need to regenerate the 
> web-rev for the Manifest fix after this gets pushed
>  
> I am running Mach5 tiers 1-3 now and will let you know when it completes
>  
>  
> 
> 
> On Feb 20, 2020, at 8:33 AM, Langer, Christoph  > wrote:
>  
> Hi,
> 
> please review this cleanup change to remove the ExistingChannelCloser 
> facility in zipfs.
> 
> Some technical details about why this existed can be found in this mailing 
> list thread, where Sherman explained its original purpose: 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059839.html 
> .
>  At that time we didn't dare to remove it and deferred to further 
> investigation.
> 
> Now, when looking at the close() implementation of ZipFileSystem during 
> review of JDK-8225507, I had a closer look to exChannelCloser as well and 
> found that it should definitely be removed. Its original purpose was to keep 
> a backing file and channel for InputStreams that were still open while the 
> sync() method was called to update a zip file on disk. However, in its 
> current state, a zip file is only synced to disk by sync() during close(), at 
> a time when all InputStreams should already be closed.
> 
> Closing of the streams is done at the beginning of method close(), in line 
> 466ff. Closing of any open InputStream would remove it from the list named 
> "streams". After that, it must be assured that no InputStreams get created, 
> of course. This is accomplished by calling ensureOpen() under readLock, 
> whenever an InputStream shall be created. The first step of close() though is 
> to set the ZipFileSystem to state "closed", so these checks should all fail. 
> I have, however, found one location where this ensureOpen check was missing 
> and added it.
> 
> jdk/nio/zipfs tests still pass.
> 
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8239556.0/ 
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8239556 
> 
> 
> Thanks
> Christoph
> 
>  
>  
> 
>  Lance Andersen| 
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
>  
> 
> 
>  

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Jpackage launcher - ways override default launcher?

[Eva Taivalsaari]

Hello!

I've been testing out jpackage with a variety of javafx applications, and
for the most part am happy with it. However, I have one question.

Is there any way currently to override the default launcher created by
jpackage entirely with a custom launcher script? And if not, are there
plans to include such a feature?

I know jpackage allows manipulating the default launcher by passing it
command line arguments, and that I can add a custom secondary launcher
script with a different name if I want to, but neither of these options are
sufficient for my use case.

I need my main launching script to calculate certain JVM arguments based on
the user's system, as well as to run an AppleScript for some choices that
are made before launching the application. Currently, I don't see any way
of doing that with jpackage, since the JVM args need to be known at the
time the app image is created.

Best regards,
Eva Taivalsaari

Reminder about JDK-8148937 (Compact Strings for StringJoiner)

[Сергей Цыпанов]

Hello,

previous week I wrote a couple of messages regarding integration of JEP 254 
into j.u.StringJoiner.
As Brent Christian pointed out [1], there's already existing JDK-8148937 [2] 
along with the patch [3].
In comments of it was pointed out, that the patch applied in 2016 
brought some regression [4] due to JDK-8149758 [5].

I've applied the same patch to existing code base of JDK 14 and got the results 
below.
Now we have only 1 case with regression within 8% of baseline. In all other 
cases we
for sure have measurable improvement.

StringJoiner is widey used in Collectors.joining(), String.join(), and as new 
StringJoiner
in many places (38 usages in java.base). In turn Collectors.joining() is used 
e.g. in j.l.Class, j.l.r.Executable and in other places (13 usages in 
java.base, 8 in jdk.compiler).

So my question is whether we can apply Alexey's patch for JDK 14 or JDK 15?

1. 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-February/064780.html
2. https://bugs.openjdk.java.net/browse/JDK-8148937
3. http://cr.openjdk.java.net/~shade/8148937/webrev.01/
4. http://cr.openjdk.java.net/~shade/8148937/notes.txt
5. https://bugs.openjdk.java.net/browse/JDK-8149758

count   latin   lenOriginalPatched

sj  1true 1 27.7 ±  1.2  19.7 ±  0.2   ns/op
sj  1true 5 27.7 ±  0.1  19.8 ±  0.0   ns/op
sj  1true10 29.0 ±  0.6  20.1 ±  0.1   ns/op
sj  1true   100 51.4 ±  0.1  32.9 ±  0.1   ns/op
sj  5true 1 71.1 ±  0.4  53.8 ±  0.1   ns/op
sj  5true 5 79.5 ±  0.3  54.8 ±  0.1   ns/op
sj  5true10 81.2 ±  0.3  56.1 ±  0.1   ns/op
sj  5true   100150.0 ±  0.4  78.5 ±  0.3   ns/op
sj 10true 1145.5 ±  0.7 127.1 ±  0.5   ns/op
sj 10true 5160.0 ±  0.8 130.8 ±  0.3   ns/op
sj 10true10165.6 ±  0.4 138.9 ±  0.4   ns/op
sj 10true   100340.2 ±  1.2 175.2 ±  0.7   ns/op
sj100true 1   1114.3 ± 15.91197.4 ± 34.0   ns/op !
sj100true 5   1184.2 ± 18.51159.0 ± 32.3   ns/op
sj100true10   1345.8 ± 13.91233.5 ± 32.9   ns/op
sj100true   100   3156.1 ± 19.11727.9 ± 37.6   ns/op

sj  1   false 1 33.5 ±  0.4  19.6 ±  0.0   ns/op
sj  1   false 5 33.3 ±  0.3  20.1 ±  0.1   ns/op
sj  1   false10 33.8 ±  0.1  20.6 ±  0.0   ns/op
sj  1   false   100 57.5 ±  0.2  39.6 ±  0.1   ns/op
sj  5   false 1 82.9 ±  0.5  64.2 ±  1.3   ns/op
sj  5   false 5 86.0 ±  0.5  66.6 ±  0.8   ns/op
sj  5   false10 96.9 ±  0.5  76.9 ±  0.7   ns/op
sj  5   false   100224.2 ±  0.6 125.1 ±  0.6   ns/op
sj 10   false 1165.7 ±  0.9 152.1 ±  1.0   ns/op
sj 10   false 5178.4 ±  0.7 163.7 ±  0.9   ns/op
sj 10   false10191.7 ±  0.9 172.5 ±  0.8   ns/op
sj 10   false   100534.4 ±  1.4 305.2 ±  1.1   ns/op
sj100   false 1   1435.9 ±  9.51333.0 ±  4.7   ns/op
sj100   false 5   1618.5 ± 14.31379.7 ± 10.4   ns/op
sj100   false10   1898.1 ±  9.41531.5 ± 13.7   ns/op
sj100   false   100   4247.4 ± 60.72423.8 ± 11.9   ns/op

sj:·g.a.r.n 1true 1124.0 ±  4. 0  96.0 ±  0.0   B/op
sj:·g.a.r.n 1true 5128.0 ±  0. 0  96.0 ±  0.0   B/op
sj:·g.a.r.n 1true10144.0 ±  0. 0 104.0 ±  0.0   B/op
sj:·g.a.r.n 1true   100416.0 ±  0. 0 192.0 ±  0.0   B/op
sj:·g.a.r.n 5true 1144.0 ±  0. 0  96.0 ±  0.0   B/op
sj:·g.a.r.n 5true 5200.0 ±  0. 0 120.0 ±  0.0   B/op
sj:·g.a.r.n 5true10272.0 ±  0. 0 144.0 ±  0.0   B/op
sj:·g.a.r.n 5true   100   1632.0 ±  0. 0 592.0 ±  0.0   B/op
sj:·g.a.r.n10true 1256.0 ±  0. 0 184.0 ±  0.0   B/op
sj:·g.a.r.n10true 5376.0 ±  0. 0 224.0 ±  0.0   B/op
sj:·g.a.r.n10true10520.0 ±  0. 0 272.0 ±  0.0   B/op
sj:·g.a.r.n10true   100   3224.1 ±  0. 01168.0 ±  0.0   B/op
sj:·g.a.r.n   100true 1   1752.1 ±  5. 41244.1 ±  9.5   B/op
sj:·g.a.r.n   100true 5   2952.2 ±  5. 41627.3 ±  7.6   B/op
sj:·g.a.r.n   100true10   .4 ±  4. 02140.2 ±  9.5   B/op
sj:·g.a.r.n   100true   100  31445.6 ±  4. 0   11135.0 ±  9.3   B/op

sj:·g.a.r.n 1   false 1144.0 ±  0. 0  96.0 ±  0.0   B/op
sj:·g.a.r.n 1   false 5160.0 ±  0. 0 104.0 ±  0.0   B/op
sj:·g.a.r.n 1   false10184.0 ±  0. 0 112.0 ±  0.0   B/op

RE: RFR [XS]: 8238947: tools/jpackage tests fail with old rpmbuild versions

[Langer, Christoph]

Hi Matthias,

I think this is good.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Donnerstag, 20. Februar 2020 14:15
> To: core-libs-dev@openjdk.java.net
> Subject: [CAUTION] RFR [XS]: 8238947: tools/jpackage tests fail with old
> rpmbuild versions
> 
> Hello, please review this small change adjusting the minimal rpmbuild version
> of tools/jpackage tests .
> 
> Currently the tools/jpackage tests, for example
> jdk/test/jdk/tools/jpackage/linux/AppCategoryTest.java fail on older distros
> with rpmbuild tools < 4.10 .
> This can be seen on Suse Linux (SLES11) with rpmbuild 4.4 and on RedHat 6
> with rpmbuild 4.8 .
> For example, rpmbuild returns an unexpected "rpmbuild: no spec files given
> for build" when calling :
> 
> > rpmbuild --eval=%{_target_cpu}
> > x86_64
> > rpmbuild: no spec files given for build
> 
> Probably we should check for at least rpmbuild version 4.10 when executing
> the tests (on Suse Linux 12 and on RedHat 7+ we have rpmbuild 4.11 or
> higher).
> 
> 
> Bug/webrev :
> 
> https://bugs.openjdk.java.net/browse/JDK-8238947
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8238947.0/
> 
> Thanks, Matthias

RE: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in zipfs implementation

[Langer, Christoph]

Hi Lance,

thanks for reviewing. Any results from Mach5? I'll push it after I get your ok.

The manifest fix will still apply after this - but you wanted to update the 
webrev anyway with some test updates, right?

When both of these fixes are in, I'll look at augmenting Jai's current proposal 
for JDK-8225507 to overhaul exception handling in close()().

Cheers
Christoph



From: Lance Andersen 
Sent: Donnerstag, 20. Februar 2020 20:00
To: Langer, Christoph 
Cc: nio-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Subject: Re: RFR 8239556: (zipfs) remove ExistingChannelCloser facility in 
zipfs implementation

Hi Christoph,

This looks good and thank you for tackling. We will need to regenerate the 
web-rev for the Manifest fix after this gets pushed

I am running Mach5 tiers 1-3 now and will let you know when it completes




On Feb 20, 2020, at 8:33 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi,

please review this cleanup change to remove the ExistingChannelCloser facility 
in zipfs.

Some technical details about why this existed can be found in this mailing list 
thread, where Sherman explained its original purpose: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059839.html. 
At that time we didn't dare to remove it and deferred to further investigation.

Now, when looking at the close() implementation of ZipFileSystem during review 
of JDK-8225507, I had a closer look to exChannelCloser as well and found that 
it should definitely be removed. Its original purpose was to keep a backing 
file and channel for InputStreams that were still open while the sync() method 
was called to update a zip file on disk. However, in its current state, a zip 
file is only synced to disk by sync() during close(), at a time when all 
InputStreams should already be closed.

Closing of the streams is done at the beginning of method close(), in line 
466ff. Closing of any open InputStream would remove it from the list named 
"streams". After that, it must be assured that no InputStreams get created, of 
course. This is accomplished by calling ensureOpen() under readLock, whenever 
an InputStream shall be created. The first step of close() though is to set the 
ZipFileSystem to state "closed", so these checks should all fail. I have, 
however, found one location where this ensureOpen check was missing and added 
it.

jdk/nio/zipfs tests still pass.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8239556.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8239556

Thanks
Christoph

[cid:image001.gif@01D5E896.DE3E9740]

Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com