RFR: JDK-8230629: jpackage signing on macOS does not work as expected

[Alexander Matveev]

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


- Binaries in runtime and Frameworks will not be signed directly using 
user provided certificate.
- libapplauncher.dylib will be signed with user provided certificate 
only if it is unsigned.
- When signing is enabled app and pkg will be signed, but not dmg. App 
inside pkg and dmg will be signed as well.


[1] https://bugs.openjdk.java.net/browse/JDK-8230629

[2] http://cr.openjdk.java.net/~almatvee/8230629/webrev.00/

Thanks,
Alexander

RFR: JDK-8230653: jpackage error on macOS system without xcode

[Alexander Matveev]

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


- Error mentioned in bug report was due to missing Xcode when running 
SetFile. Fixed by not running SetFile if Xcode is not available. This 
tool used to set custom icon on DMG file and we do not consider it as 
real error. Verbose output will indicate that custom icon is skipped for 
DMG.
- Signing will fail if Xcode is not installed. Added check for Xcode 
availability if signing is requested. If Xcode is not available and 
signing is specified we will fail with error.


[1] https://bugs.openjdk.java.net/browse/JDK-8230653

[2] http://cr.openjdk.java.net/~almatvee/8230653/webrev.00

Thanks,
Alexander

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Martin Buchholz]

We're mostly in agreement.
(Also, I'm not actually an ldap reviewer.)

On Wed, Sep 11, 2019 at 11:02 AM Pavel Rappo  wrote:

> I agree that this example [1] looks readable (depending on a reader, of
> course). I think it looks readable mostly because it is very explicit.
> However, in a domain other than concurrency we are not usually concerned
> with this level of detail.


I think you *are* in the domain of concurrency!


> a. is not using any synchronization aid to make sure both threads wait for
> each other (probably, the timeout value accommodates that)
>

Thread.join is a synchronization aid!  But there's no shortage of others.
We typically use a CountDownLatch to synchronize with another thread.


> b. uses a number of tiny low-level methods like newStartedThread,
> awaitTermination, millisElapsedSince, manual time assertions, etc.
> c. has assertions spread across 2 threads
>

I don't see that as a problem, as long as every assertion failure
propagates properly to become a test failure.


>
> (b) probably allows you to reuse components in places unrelated to
> timeouts. At the same time you don't seem to have any higher-level reusable
> component (i.e. my guess is that this code is more or less repeated in
> other places in that test suite, which is not necessarily a bad thing).
>
> However, I fully agree with you that this functionality should be a
> utility method of the test library.
>
> -
> [1] I assume this is an excerpt from CountDownLatchTest.java. If so, then
> for the reader's convenience this method could be seen in its context at
> http://hg.openjdk.java.net/jdk/jdk/file/d52f77f0acb5/test/jdk/java/util/concurrent/tck/CountDownLatchTest.java#l198
> [2] I'm not saying that those things are wrong or that I disagree with any
> of that. It's just an observation from reading this example.
>
>

Re: RFR: JDK-8229779: Shortcut creation policy

[Alexey Semenyuk]

SimplePackageTest.java:
I'd suggest to put "Installer should not create any shortcuts" in the 
description or simply remove notice about shortcuts.


Did you omit adding shortcuts to LinuxDebBundler.java on purpose?

- Alexey

On 9/11/2019 9:07 PM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


This fix:

1.) adds the new option --linux-shortcut, and now only creates a 
shortcut on linux if specified


2.) only creates a shortcut on windows if win-menu or win-shortcut is 
specified.


/Andy


[1] https://bugs.openjdk.java.net/browse/JDK-8229779

[2] http://cr.openjdk.java.net/~herrick/8229779

Re: RFR: JDK-8229779: Shortcut creation policy

[Alexander Matveev]

Looks good.

On 9/11/2019 6:07 PM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


This fix:

1.) adds the new option --linux-shortcut, and now only creates a 
shortcut on linux if specified


2.) only creates a shortcut on windows if win-menu or win-shortcut is 
specified.


/Andy


[1] https://bugs.openjdk.java.net/browse/JDK-8229779

[2] http://cr.openjdk.java.net/~herrick/8229779

RFR: JDK-8229779: Shortcut creation policy

[Andy Herrick]

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


This fix:

1.) adds the new option --linux-shortcut, and now only creates a 
shortcut on linux if specified


2.) only creates a shortcut on windows if win-menu or win-shortcut is 
specified.


/Andy


[1] https://bugs.openjdk.java.net/browse/JDK-8229779

[2] http://cr.openjdk.java.net/~herrick/8229779

Re: RFR [14] 8151678: com/sun/jndi/ldap/LdapTimeoutTest.java failed due to timeout on DeadServerNoTimeoutTest is incorrect

[Pavel Rappo]

Martin,

> On 10 Sep 2019, at 17:40, Martin Buchholz  wrote:
> 
> Here's a canonical example of an "expected timeout" test that seems natural 
> and readable to me.
> timeoutMillis() returns 12ms, adjusted by timeout factor.  Ldap might need a 
> bigger value.
> 
> 
> /**
>  * timed await times out if not counted down before timeout
>  */
> public void testAwaitTimeout() throws InterruptedException {
> final CountDownLatch l = new CountDownLatch(1);
> Thread t = newStartedThread(new CheckedRunnable() {
> public void realRun() throws InterruptedException {
> assertEquals(1, l.getCount());
> 
> long startTime = System.nanoTime();
> assertFalse(l.await(timeoutMillis(), MILLISECONDS));
> assertTrue(millisElapsedSince(startTime) >= timeoutMillis());
> 
> assertEquals(1, l.getCount());
> }});
> 
> awaitTermination(t);
> assertEquals(1, l.getCount());
> }


I agree that this example [1] looks readable (depending on a reader, of 
course). I think it looks readable mostly because it is very explicit. However, 
in a domain other than concurrency we are not usually concerned with this level 
of detail. In such cases I would prefer to have a small set of focused methods 
that would allow me to explain my intentions succinctly and only once, taking 
care of all the low-level details.

As for the internal mechanics, I can see that this example [2]:

a. is not using any synchronization aid to make sure both threads wait for each 
other (probably, the timeout value accommodates that)
b. uses a number of tiny low-level methods like newStartedThread, 
awaitTermination, millisElapsedSince, manual time assertions, etc.
c. has assertions spread across 2 threads

(b) probably allows you to reuse components in places unrelated to timeouts. At 
the same time you don't seem to have any higher-level reusable component (i.e. 
my guess is that this code is more or less repeated in other places in that 
test suite, which is not necessarily a bad thing).

However, I fully agree with you that this functionality should be a utility 
method of the test library.

-
[1] I assume this is an excerpt from CountDownLatchTest.java. If so, then for 
the reader's convenience this method could be seen in its context at 
http://hg.openjdk.java.net/jdk/jdk/file/d52f77f0acb5/test/jdk/java/util/concurrent/tck/CountDownLatchTest.java#l198
[2] I'm not saying that those things are wrong or that I disagree with any of 
that. It's just an observation from reading this example.

Re: 8230342: LineNumberReader.getLineNumber() returns inconsistent results after EOF

[Roger Riggs]

Looks good.

Thanks for the alternative investigations,  Roger


On 9/11/19 12:58 PM, Brian Burkhalter wrote:

Although I rather like [1] it is probably too expensive to use a lambda just to 
increment the line number. Therefore I am proposing to modify [2] to replace 
the AtomicBoolean with a boolean[] as in [3].

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8230342/webrev.01/
[2] http://cr.openjdk.java.net/~bpb/8230342/webrev.00/
[3] http://cr.openjdk.java.net/~bpb/8230342/webrev.02/


On Sep 10, 2019, at 11:46 AM, Brian Burkhalter  
wrote:

Hi Daniel,

Thanks for the idea. It seems a little strange to me though to have the 
incrementing being done in a separate method. It feels a little disconnected.

I wrote up an alternative [1] which instead passes a functional interface to 
the readLine() in BufferedReader. This eliminates creating an AtomicBoolean and 
avoids using a boolean[] parameter like a pseudo-pointer.

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8230342/webrev.01/


On Sep 10, 2019, at 7:57 AM, Daniel Fuchs  wrote:

On 09/09/2019 15:35, Roger Riggs wrote:

- Is the use of AtomicBoolean due to concurrency concerns?
  If not, a new boolean[1] would be less overhead

Alternatively, BufferedReader could define an empty package
method called e.g.

  void endOfLine() { };

that LineNumberReader could override to increment lineNumber.

So:

351 if (term != null) term.set(true);

would simply become

351 endOfLine();

which would be a no-op for BufferedReader but would increment
lineNumber for LineNumberReader.

Wouldn't that work too?

Re: 8230342: LineNumberReader.getLineNumber() returns inconsistent results after EOF

[Brian Burkhalter]

Although I rather like [1] it is probably too expensive to use a lambda just to 
increment the line number. Therefore I am proposing to modify [2] to replace 
the AtomicBoolean with a boolean[] as in [3].

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8230342/webrev.01/
[2] http://cr.openjdk.java.net/~bpb/8230342/webrev.00/
[3] http://cr.openjdk.java.net/~bpb/8230342/webrev.02/

> On Sep 10, 2019, at 11:46 AM, Brian Burkhalter  
> wrote:
> 
> Hi Daniel,
> 
> Thanks for the idea. It seems a little strange to me though to have the 
> incrementing being done in a separate method. It feels a little disconnected.
> 
> I wrote up an alternative [1] which instead passes a functional interface to 
> the readLine() in BufferedReader. This eliminates creating an AtomicBoolean 
> and avoids using a boolean[] parameter like a pseudo-pointer.
> 
> Thanks,
> 
> Brian
> 
> [1] http://cr.openjdk.java.net/~bpb/8230342/webrev.01/
> 
>> On Sep 10, 2019, at 7:57 AM, Daniel Fuchs  wrote:
>> 
>> On 09/09/2019 15:35, Roger Riggs wrote:
>>> - Is the use of AtomicBoolean due to concurrency concerns?
>>>  If not, a new boolean[1] would be less overhead
>> 
>> Alternatively, BufferedReader could define an empty package
>> method called e.g.
>> 
>>  void endOfLine() { };
>> 
>> that LineNumberReader could override to increment lineNumber.
>> 
>> So:
>> 
>> 351 if (term != null) term.set(true);
>> 
>> would simply become
>> 
>> 351 endOfLine();
>> 
>> which would be a no-op for BufferedReader but would increment
>> lineNumber for LineNumberReader.
>> 
>> Wouldn't that work too?
> 

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Pavel Rappo]

> On 11 Sep 2019, at 16:55, Alan Bateman  wrote:
> 
> I don't think the jndi-dns docs page is in the jdk repo.

Could you kindly point me to the location of the current jndi-dns docs page? I 
can't seem to be able to even find anything related to that on the web. You 
might want to try to use your favorite search engine.

> 
> 
> However, for the changes under discussion here then I don't think it's 
> unreasonable to provide an updated description of the timeout property.

It might be reasonable. What wording would you suggest? To me it already 
*vaguely* addresses the TCP case by saying that "Each lookup is initially 
performed using UDP. If the response is too long to be returned in a UDP packet 
without being truncated, the lookup is repeated using TCP."

-Pavel

Re: RFR: Request for sponsor : 8230436: String.stripIndent() javadoc unclear about LF/CR at end of string

[Andrew Leonard]

Thanks Jim,
no worries time wise.
Cheers
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:   Jim Laskey 
To: Andrew Leonard 
Cc: core-libs-dev@openjdk.java.net
Date:   10/09/2019 19:05
Subject:Re: RFR: Request for sponsor : 8230436: 
String.stripIndent() javadoc unclear about LF/CR at end of string



Andrew,

I'll take it on, but it will have to go through a CSR review and thus will 
take some time.

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

Cheers,

-- Jim




On Sep 6, 2019, at 5:55 AM, Andrew Leonard  
wrote:

Hi, 
Anyone able to review please? 
Thanks 
Andrew 

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 




From:Andrew Leonard/UK/IBM 
To:core-libs-dev@openjdk.java.net 
Cc:james.las...@oracle.com 
Date:02/09/2019 17:10 
Subject:RFR: Request for sponsor : 8230436: String.stripIndent() 
javadoc unclear about LF/CR at end of string 


Hi, 
Can I get people's views and sponsor on a small update to the javadoc for 
String.stripIndent() to make the \n end of block special case more clear 
please? 
I've done a proposed patch here: 
http://cr.openjdk.java.net/~aleonard/8230436/webrev.00/ 
Thanks 
Andrew 

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
internet email: andrew_m_leon...@uk.ibm.com 

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



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



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(s): 8228580: DnsClient TCP socket timeout

[Alan Bateman]

On 11/09/2019 16:16, Pavel Rappo wrote:



On 11 Sep 2019, at 16:10, Alan Bateman  wrote:

I assume extending the timeout to TCP will require at least some minimal 
updates to the descriptions.

Which descriptions do you have in mind? I cannot seem to find that text 
anywhere in the current repo (jdk/jdk)
I don't think the jndi-dns docs page is in the jdk repo. Since JDK 9, a 
good place for service providers to document capability and 
configuration is their module description and I think at least some of 
the documentation from that page should move into the jdk.naming.dns 
module description (that's for another issue of course). However, for 
the changes under discussion here then I don't think it's unreasonable 
to provide an updated description of the timeout property.


-Alan

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Pavel Rappo]

> On 11 Sep 2019, at 16:10, Alan Bateman  wrote:
> 
> I assume extending the timeout to TCP will require at least some minimal 
> updates to the descriptions. 

Which descriptions do you have in mind? I cannot seem to find that text 
anywhere in the current repo (jdk/jdk)

$ hg tip
changeset:   56237:cddef3bde924
tag: tip
user:lkorinth
date:Wed Sep 11 14:16:30 2019 +0200
summary: 8230398: Remove NULL checks before FREE_C_HEAP_ARRAY.

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Alan Bateman]

On 11/09/2019 15:56, Pavel Rappo wrote:

Sure, from 
https://docs.oracle.com/javase/8/docs/technotes/guides/jndi/jndi-dns.html:

"Each lookup is initially performed using UDP. If the response is too long to be 
returned in a UDP packet without being truncated, the lookup is repeated using TCP."

and

"com.example.jndi.dns.timeout.initial
com.example.jndi.dns.timeout.retries

These properties are used to alter the timeout-related defaults that the DNS 
provider uses when submitting UDP queries. The DNS provider submits UDP queries 
using the following exponential backoff algorithm. The provider submits a query 
to a DNS server and waits for a response to arrive within a timeout period (1 
second by default). If it receives no response within the timeout period, it 
queries the next server, and so on. If the provider receives no response from 
any server, it doubles the timeout period and repeats the process of submitting 
the query to each server, up to a maximum number of retries (4 by default).

The "com.example.jndi.dns.timeout.initial" property, if set, specifies the 
number of milliseconds to use as the initial timeout period (i.e., before any doubling). 
If this property has not been set, the default initial timeout is 1000 milliseconds.

The "com.example.jndi.dns.timeout.retries" property, if set, specifies the number of 
times to retry each server using the exponential backoff algorithm described previously. If 
this property has not been set, the default number of retries is 4."

I cannot seem to find a newer version of that document though.

I assume extending the timeout to TCP will require at least some minimal 
updates to the descriptions. That will help reviewers and help decide if 
a CSR is needed or not. Ideally the authoritative descriptions of these 
properties would be in the javadoc, probably in the jdk.naming.dns 
module description.


-Alan

Re: Feedback JEP 343: Packaging Tool

[Alexey Semenyuk]

Hi Heiko,

Thank you for giving a try to jpackage and for your feedback!

Please find comments inlined.

On 9/10/2019 4:55 AM, Heiko Wagner wrote:

I am sending this feedback as suggested by the jpackage project
(https://jdk.java.net/jpackage/). I hope this is considered helpful
information.

I have tried the current build "14-jpackage+1-35" on Windows. It pretty
much worked out of the box for me. Here are a few observations I make:

- creating a image of the application is great for building protable
applications. Sometime it is realy great to have the application on a
thumb drive an just run it on any machine; currently i do this manually
via jlink and use launch4j as a native launcher

- MSI installation packages are great when deploying a application into
a controlled it infrastructure, but in turn impose some restictions e.g.
on application updates
Correct. That is why jpackage supports creation of an application image 
not bundled in MSI on Windows to give maximum flexibility.



- my application is currently a portable app and does not use any native
installer like MSI. Automatic updates are handled by the application by
just in place updating the jar files. Deploying the application as a MSI
would require to change the update behaviour. Even if automatic updates
are out of the scope of the current JEP it would be helpful to have a
common solution for this in the long run

- Currently my app has a splash screen, since launch4j has support for a
native splash screen. I have no tried it on jpackage, but the JEP states
that there is no support for native splash screen.

That is correct, jpackage is not offering native splash screen support.

Does this also mean that the functionality of "-splash: fileName" and
its manifest file counter part will not work when using the launcher
generated jpackage?
Right now it is not working as far as I can say from running the simple 
test. This looks like a bug in app launcher generated by jpackage. I've 
filed https://bugs.openjdk.java.net/browse/JDK-8230862 to track this.


- Wher creating a image on Windows pretty much all .DLL from the bin
folder in the runtime are duplicated into the application folder. Is
this intended?
App launcher is linked dynamically with MS run-time libs so we have MS 
run-time dlls in the folder next to the launcher. There is no bold 
reason to keep it this way, we can link app launcher statically with MS 
run-time and get rid of all these dlls.

Created https://bugs.openjdk.java.net/browse/JDK-8230863 to track this.

- Alexey

When using the generic java.exe launcher it is not
required to this. It just works fine with all .DLL/.EXE files in the
runtime bin folder

Thanks for making such a great tool. Keep up the good work.

Regards,

Heiko

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Pavel Rappo]

Sure, from 
https://docs.oracle.com/javase/8/docs/technotes/guides/jndi/jndi-dns.html:

"Each lookup is initially performed using UDP. If the response is too long to 
be returned in a UDP packet without being truncated, the lookup is repeated 
using TCP."

and

"com.example.jndi.dns.timeout.initial
com.example.jndi.dns.timeout.retries

These properties are used to alter the timeout-related defaults that the DNS 
provider uses when submitting UDP queries. The DNS provider submits UDP queries 
using the following exponential backoff algorithm. The provider submits a query 
to a DNS server and waits for a response to arrive within a timeout period (1 
second by default). If it receives no response within the timeout period, it 
queries the next server, and so on. If the provider receives no response from 
any server, it doubles the timeout period and repeats the process of submitting 
the query to each server, up to a maximum number of retries (4 by default).

The "com.example.jndi.dns.timeout.initial" property, if set, specifies the 
number of milliseconds to use as the initial timeout period (i.e., before any 
doubling). If this property has not been set, the default initial timeout is 
1000 milliseconds.

The "com.example.jndi.dns.timeout.retries" property, if set, specifies the 
number of times to retry each server using the exponential backoff algorithm 
described previously. If this property has not been set, the default number of 
retries is 4."

I cannot seem to find a newer version of that document though.

> On 11 Sep 2019, at 14:58, Alan Bateman  wrote:
> 
> On 11/09/2019 13:26, Pavel Rappo wrote:
>> I'm happy with the overall changeset. I have (once again) made some tiny 
>> changes, you can see them here:
>> 
>> http://cr.openjdk.java.net/~prappo/8228580/webrev.02/
>> 
>> If you are okay with them, then we wait for a *R*eviewer. If the Reviewer(s) 
>> are okay with them, we push. For the record, I'm not really happy with how 
>> we used the DNSTestBase/TestBase infrastructure, however I'm totally fine 
>> with the retrying logic.
>> 
> Would it be possible to write a clear description for the 
> com.sun.jndi.dns.timeout and com.sun.jndi.dns.retries properties? The current 
> description in the JNDI docs describes them for UDP only. I realize this is 
> may be out of date but new descriptions would be useful when the docs are 
> updated. Also I think would be useful for potential Reviewers.
> 
> -Alan

Re: RFR: jsr166 integration 2019-09

[Alan Bateman]

On 09/09/2019 16:06, Martin Buchholz wrote:

:

Another round of stress testing at Google allowed us to fix a handful of
very rare test failures.

8227235: rare failures in testForkHelpQuiesce tck tests
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ForkJoin-quiesce/index.html
https://bugs.openjdk.java.net/browse/JDK-8227235

8221168: java/util/concurrent/CountDownLatch/Basic.java fails
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/CountDownLatch-Basic/index.html
https://bugs.openjdk.java.net/browse/JDK-8221168

8145138: CyclicBarrier/Basic.java failed with "3 not equal to 4"
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/CyclicBarrier-Basic/index.html
https://bugs.openjdk.java.net/browse/JDK-8145138

8225490: Miscellaneous changes imported from jsr166 CVS 2019-09
https://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html
https://bugs.openjdk.java.net/browse/JDK-8225490
I read through the test updates and other misc. changes and don't see 
anything obviously wrong (the discussion in JIRA on the test failures 
was useful). At some point I could imagine Joe suggesting add "@key 
randomness" to more of these tests.


-Alan.

Re: [14] RFR: 8230136: DateTimeFormatterBuilder.FractionPrinterParser#parse fails to verify minWidth

[Roger Riggs]

+1

On 9/11/19 8:48 AM, Stephen Colebourne wrote:

+1

On Wed, 11 Sep 2019 at 13:38,  wrote:

Hi Stephen,

I confirmed that issuing parseStrict() was irrelevant. The incorrect
text won't throw the exception in "lenient" mode as well. In addition,
"builder" is always instantiated in AbstractTestPrinterParser on each
TestNG invocation and "strict" is the default mode. Thus I did not
explicitly issue parseStrict() in the test.

BTW, I found typos wrt the default parse mode. Included the corrections
in this webrev as well:

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

Naoto

On 9/11/19 2:01 AM, Stephen Colebourne wrote:

The bug references parseStrict() but the test does not. Is the builder
already set to parseStrict() ? Anyway, if the bug is to be clearly
squished, parseStrict() should appear somewhere.
Stephen

On Tue, 10 Sep 2019 at 23:42, Joe Wang  wrote:

+1, looks good to me.

Best regards,
Joe

On 9/10/19 2:20 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8230136/webrev.00/

The fix is to correct the condition of the invalid case, as suggested
in the bug report.

Naoto

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Alan Bateman]

On 11/09/2019 13:26, Pavel Rappo wrote:

I'm happy with the overall changeset. I have (once again) made some tiny 
changes, you can see them here:

 http://cr.openjdk.java.net/~prappo/8228580/webrev.02/

If you are okay with them, then we wait for a *R*eviewer. If the Reviewer(s) 
are okay with them, we push. For the record, I'm not really happy with how we 
used the DNSTestBase/TestBase infrastructure, however I'm totally fine with the 
retrying logic.

Would it be possible to write a clear description for the 
com.sun.jndi.dns.timeout and com.sun.jndi.dns.retries properties? The 
current description in the JNDI docs describes them for UDP only. I 
realize this is may be out of date but new descriptions would be useful 
when the docs are updated. Also I think would be useful for potential 
Reviewers.


-Alan

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Milan Mimica]

Hi Pavel

Your changes look good to me.

On Wed, 11 Sep 2019 at 14:26, Pavel Rappo  wrote:
>
> I'm happy with the overall changeset. I have (once again) made some tiny 
> changes, you can see them here:
>
> http://cr.openjdk.java.net/~prappo/8228580/webrev.02/
>
> If you are okay with them, then we wait for a *R*eviewer. If the Reviewer(s) 
> are okay with them, we push. For the record, I'm not really happy with how we 
> used the DNSTestBase/TestBase infrastructure, however I'm totally fine with 
> the retrying logic.
>
> Test results are pending.
>
> -Pavel
>
> > On 10 Sep 2019, at 16:33, Milan Mimica  wrote:
> >
> >>> On 5 Sep 2019, at 16:02, Pavel Rappo  wrote:
> >>>
> >>> I think we are almost there. What do you think of the following 
> >>> incremental (i.e. on top of your latest webrev) change?
> >>>
> >>>   http://cr.openjdk.java.net/~prappo/8228580/webrev.01/
> >>>
> >>> I fixed a couple of trivial typos and addressed the socket relinquishing 
> >>> issue. Initializing a socket is not an atomic "all-or-nothing" operation 
> >>> now. Someone needs to take care of the socket in case things go not as 
> >>> planned.
> >
> > Right. Thanks. Here is the merged version:
> > http://cr.openjdk.java.net/~mmimica/8228580/webrev.02/
> > Plus, I have added TCP server init retry code from Chris. Works fine
> > without changes to TestBase.
> >
> >
> > --
> > Milan Mimica
>


-- 
Milan Mimica

Re: [14] RFR: 8230136: DateTimeFormatterBuilder.FractionPrinterParser#parse fails to verify minWidth

[Stephen Colebourne]

+1

On Wed, 11 Sep 2019 at 13:38,  wrote:
>
> Hi Stephen,
>
> I confirmed that issuing parseStrict() was irrelevant. The incorrect
> text won't throw the exception in "lenient" mode as well. In addition,
> "builder" is always instantiated in AbstractTestPrinterParser on each
> TestNG invocation and "strict" is the default mode. Thus I did not
> explicitly issue parseStrict() in the test.
>
> BTW, I found typos wrt the default parse mode. Included the corrections
> in this webrev as well:
>
> http://cr.openjdk.java.net/~naoto/8230136/webrev.01/
>
> Naoto
>
> On 9/11/19 2:01 AM, Stephen Colebourne wrote:
> > The bug references parseStrict() but the test does not. Is the builder
> > already set to parseStrict() ? Anyway, if the bug is to be clearly
> > squished, parseStrict() should appear somewhere.
> > Stephen
> >
> > On Tue, 10 Sep 2019 at 23:42, Joe Wang  wrote:
> >>
> >> +1, looks good to me.
> >>
> >> Best regards,
> >> Joe
> >>
> >> On 9/10/19 2:20 PM, naoto.s...@oracle.com wrote:
> >>> Hello,
> >>>
> >>> Please review the fix to the following issue:
> >>>
> >>> https://bugs.openjdk.java.net/browse/JDK-8230136
> >>>
> >>> The proposed changeset is located at:
> >>>
> >>> http://cr.openjdk.java.net/~naoto/8230136/webrev.00/
> >>>
> >>> The fix is to correct the condition of the invalid case, as suggested
> >>> in the bug report.
> >>>
> >>> Naoto
> >>

Re: [14] RFR: 8230136: DateTimeFormatterBuilder.FractionPrinterParser#parse fails to verify minWidth

[naoto . sato]

Hi Stephen,

I confirmed that issuing parseStrict() was irrelevant. The incorrect 
text won't throw the exception in "lenient" mode as well. In addition, 
"builder" is always instantiated in AbstractTestPrinterParser on each 
TestNG invocation and "strict" is the default mode. Thus I did not 
explicitly issue parseStrict() in the test.


BTW, I found typos wrt the default parse mode. Included the corrections 
in this webrev as well:


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

Naoto

On 9/11/19 2:01 AM, Stephen Colebourne wrote:

The bug references parseStrict() but the test does not. Is the builder
already set to parseStrict() ? Anyway, if the bug is to be clearly
squished, parseStrict() should appear somewhere.
Stephen

On Tue, 10 Sep 2019 at 23:42, Joe Wang  wrote:


+1, looks good to me.

Best regards,
Joe

On 9/10/19 2:20 PM, naoto.s...@oracle.com wrote:

Hello,

Please review the fix to the following issue:

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

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8230136/webrev.00/

The fix is to correct the condition of the invalid case, as suggested
in the bug report.

Naoto

Re: RFR(s): 8228580: DnsClient TCP socket timeout

[Pavel Rappo]

I'm happy with the overall changeset. I have (once again) made some tiny 
changes, you can see them here:

http://cr.openjdk.java.net/~prappo/8228580/webrev.02/

If you are okay with them, then we wait for a *R*eviewer. If the Reviewer(s) 
are okay with them, we push. For the record, I'm not really happy with how we 
used the DNSTestBase/TestBase infrastructure, however I'm totally fine with the 
retrying logic.

Test results are pending.

-Pavel

> On 10 Sep 2019, at 16:33, Milan Mimica  wrote:
> 
>>> On 5 Sep 2019, at 16:02, Pavel Rappo  wrote:
>>> 
>>> I think we are almost there. What do you think of the following incremental 
>>> (i.e. on top of your latest webrev) change?
>>> 
>>>   http://cr.openjdk.java.net/~prappo/8228580/webrev.01/
>>> 
>>> I fixed a couple of trivial typos and addressed the socket relinquishing 
>>> issue. Initializing a socket is not an atomic "all-or-nothing" operation 
>>> now. Someone needs to take care of the socket in case things go not as 
>>> planned.
> 
> Right. Thanks. Here is the merged version:
> http://cr.openjdk.java.net/~mmimica/8228580/webrev.02/
> Plus, I have added TCP server init retry code from Chris. Works fine
> without changes to TestBase.
> 
> 
> -- 
> Milan Mimica

RFR: 8223490: Optimize search algorithm for determining default time zone

[Seán Coffey]

The current algorithm for determining the default timezone on (non-AIX) 
unix systems goes something like :


1. If TZ environment variable is defined, use it
2. else if /etc/timezone exists, use the value contained within it
3. else if /etc/localtime exists and is a symbolic link, use the name 
pointed to
4. else if /etc/localtime is a binary, find the first identical time 
zone binary file in /usr/share/zoneinfo/


Step 4 is a bit crude in that the zoneinfo directory can contain over 
1,800 files on today's systems. I'd like to change the logic so that 
common timezones are first checked for buffer matching before a full 
directory traversal is performed. It should be a performance gain and it 
should also lead to more consistent results for reasons outlined in the 
bug report.


https://bugs.openjdk.java.net/browse/JDK-8223490
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8223490/webrev/ 



--
Regards,
Sean.

Re: [14] RFR: 8230136: DateTimeFormatterBuilder.FractionPrinterParser#parse fails to verify minWidth

[Stephen Colebourne]

The bug references parseStrict() but the test does not. Is the builder
already set to parseStrict() ? Anyway, if the bug is to be clearly
squished, parseStrict() should appear somewhere.
Stephen

On Tue, 10 Sep 2019 at 23:42, Joe Wang  wrote:
>
> +1, looks good to me.
>
> Best regards,
> Joe
>
> On 9/10/19 2:20 PM, naoto.s...@oracle.com wrote:
> > Hello,
> >
> > Please review the fix to the following issue:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8230136
> >
> > The proposed changeset is located at:
> >
> > http://cr.openjdk.java.net/~naoto/8230136/webrev.00/
> >
> > The fix is to correct the condition of the invalid case, as suggested
> > in the bug report.
> >
> > Naoto
>

Re: RFR: 8212117 : Class.forName may return a reference to a loaded but not linked Class

[serguei.spit...@oracle.com]

Hi Brent,

The updated webrev looks good to me.
At least, I do not see any issues.

Thanks,
Serguei


On 9/5/19 17:09, Brent Christian wrote:

Hi, David

On 9/5/19 12:52 AM, David Holmes wrote:


Good to see this all come together :)


:)

So to clarify for others any current caller to 
find_class_from_class_loader that passes true for "init" was already 
implicitly asking for a linked class (as you must be linked before 
you can be initialized). With that in mind I would suggest that in 
find_class_from_class_loader you add an assert:


! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name, 
jboolean init, jboolean link,
   Handle loader, Handle 
protection_domain,

   jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link 
arguments");


just to ensure the callers understand this.


I'm good with adding an assert to check for coherent arguments.

(Another interpretation is that if 'init' is set, then the 'link' 
argument is ignored, since linking is implied).


Aside: in looking at this I've spotted another existing bug. JNI 
FindClass is not specified to perform class initialization, but the 
implementation passes init=true!


OK, thanks.  I've noted this in JDK-8226540.


src/java.base/share/classes/java/lang/invoke/MethodHandles.java

I don't see the need for the new note given it already has

* @throws LinkageError if the linkage fails


(Discussed in the CSR)


src/java.base/share/classes/sun/launcher/LauncherHelper.java
... > Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a 
number of others are possible - in particular IllegalAccessError 
might occur during verification. Other than the fact a test obviously 
triggered this, it's not clear why ACE should be singled out here. ??


Also discussed in the CSR[1].


test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java

45 // public static void foo(Foo f) { }

Unclear why this exists. Also the test refers initially to class Foo 
but then uses Foo2 and Foo3. ??


I'm guessing it's just a leftover from an earlier version of the test. 
I've removed the comment, and updated the test @summary.


Serguei, anything to add?


There is also a CSR[2] in need of review.


I've added a couple of comments and will add myself as a reviewer 
once things are near finalized.


Thanks for taking a look.

Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/

-Brent

1. 
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303




The spec for the 2-arg and 3-arg Class.forName() methods states they 
will "locate, load, and link" the class, however the linking part is 
not ensured to happen.


Although the VM spec allows flexibility WRT when classes are linked, 
this is a corner where the Class.forName() spec is not being upheld. 
While this is not an issue for most usages, 8181144 [3] demonstrates 
how this can be a problem (with the debugging interface, in this case).


This fix ensures that linking happens during the course of 
Class.forName().  Class.forName() already @throws LinkageError, so 
no spec change is needed there. MethodHandles.Lookup.findClass() 
needs a small spec update, due to calling Class.forName with 
init=false,


Of course Errors are not required to be caught.  It is therefore 
possible that the new behavior could introduce previously unseen, 
possibly unhandled LinkageErrors.  A new VM flag 
(-XX:+ClassForNameDeferLinking) is introduced to restore the 
previous behavior (to keep such code running until it can be updated).


This change surfaced a couple new "A JNI error has occurred" 
situations (see 8181033[5]) in the Launcher, by way of

   test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
   test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).

The Launcher is updated to maintain non-confusing error messages :).

The new test included with this fix ensures that 8181144[3] is 
addressed.  Thanks go to Serguei Spitsyn for writing the test.


Automated corelibs and hotspot tests pass cleanly.

Thanks,
-Brent

--
1. https://bugs.openjdk.java.net/browse/JDK-8212117

2. https://bugs.openjdk.java.net/browse/JDK-8222071

3. https://bugs.openjdk.java.net/browse/JDK-8181144

4. 
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986 



5. https://bugs.openjdk.java.net/browse/JDK-8181033