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

[Rob McKenna]

This looks great to me. Thanks for taking a look at it.

-Rob

On 12/09/19 12:36, Pavel Rappo wrote:
> > On 12 Sep 2019, at 03:54, Martin Buchholz  wrote:
> > 
> > We're mostly in agreement.
> 
> Good to hear and I agree!
> 
> > On Wed, Sep 11, 2019 at 11:02 AM Pavel Rappo  > > wrote:
> > 
> >  
> > 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.
> 
> 
> I should have expressed myself more clear. I meant that the main thread and 
> the thread created (and started) by `newStartedThread` (the test tread) do 
> not wait for *each other* before they begin. This might be needed to make 
> sure that they start at the same time (loosely speaking, of course). One 
> might argue that the importance of this diminishes with the increase of the 
> timeout that the main thread uses to wait for the test thread to die and the 
> accuracy of measurements one agrees to tolerate.
> 
> I'm seeing differences in our timeout-measuring routines as just a difference 
> in tastes. Since a particular behavior here is not guaranteed anyway (as it's 
> not a hard real-time system), all we can hope for in both cases is adequate 
> timeout values and useful diagnostic output.
> 
> Thanks for patiently staying with this thread for so long, Martin.
> 
> 
>  ***
> 
> 
> Here's the updated version of the RFR based on the discussion so far:
> 
> http://cr.openjdk.java.net/~prappo/8151678/webrev.02/
> 
> For the reviewers. I totally forgot to explain why there's a multiplier of 2 
> in some of the timeout calculations. The reason is the current behavior of 
> InitialDirContext. The supplied connect timeout seems to be used twice. Once 
> for making the actual TCP connection [1] and the second time while waiting 
> for the server to respond to the BIND message [2]. Thus, the total time spent 
> in that InitialDirContext ctor may be twice the expected. I believe it's a 
> bug, but the bug that is not related to the issue in question. The current 
> issue (8151678) is about intermittent failures of LdapTimeoutTest.
> 
> -Pavel
> 
> ---
> [1] 
> http://hg.openjdk.java.net/jdk/jdk/file/79186d82463e/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#l296,
>  
> http://hg.openjdk.java.net/jdk/jdk/file/79186d82463e/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#l320
> [2] 
> http://hg.openjdk.java.net/jdk/jdk/file/79186d82463e/src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java#l154,
>  
> http://hg.openjdk.java.net/jdk/jdk/file/79186d82463e/src/java.naming/share/classes/com/sun/jndi/ldap/LdapClient.java#l365
> 

RFR: 8132359 - JarURLConnection.getJarFile() resource leak when file is not found

[Rob McKenna]

Thanks Daniel,

cc'ing core-libs-dev in case there are any objections.

-Rob

On 25/11/19 10:47, Daniel Fuchs wrote:
> Hi Rob,
> 
> That looks good to me. I wonder if that should go to corelibs for
> review as well.
> 
> The underlying issue here is that JarURLConnection open both its
> jar file and its jar file entry in its connect() method.
> However, it delegates the opening of the jar file to the cache,
> when uses cache is true.
> In this latter case, if opening the entry throws an exception,
> it can't close the jar file because the file might have come from
> the cache and it might still be used by someone else.
> But because getJarFile() calls connect() then there's no way
> to retrieve the jar file through that (failed) connection.
> 
> I agree that fixing the issue in URLClassPath is probably the
> less risky.
> 
> I see that your test caters for all possible setting of uses cache
> and combination of success/failure when opening the jar entry,
> so this give me confidence that the fix is working as intended.
> 
> best regards,
> 
> -- daniel
> 
> 
> On 24/11/2019 15:33, Rob McKenna wrote:
> > Hi folks,
> > 
> > If a FileNotFoundException is thrown when attempting to load a class
> > from a jar file, a reference to the open JarFile remains even after the
> > loader is closed. The test in the webrev demonstrates the problem by
> > attempting to delete the JarFile after attempting a class load.
> > 
> > The deletion will fail as the JarFile is still in use (i.e. open)
> > despite the fact that the loader has been closed.
> > 
> > Note: the behaviour depends on the URLConnections useCaches setting so
> > the test cycles through the combinations. (this helpfully found a problem
> > with an earlier fix attempt)
> > 
> > bug: https://bugs.openjdk.java.net/browse/JDK-8132359
> > webrev: http://cr.openjdk.java.net/~robm/8132359/webrev.01/
> > 
> > Thanks,
> > 
> >  -Rob
> > 
> 

Re: RFR: 8132359 - JarURLConnection.getJarFile() resource leak when file is not found

[Rob McKenna]

I'm afraid so.

The option of changing the the JarURLConnection spec to indicate what
could happen when the class file is missing was discussed but that will
require some further thought. (and it may not necessarily result in a fix
for this problem, the assumption being that behaviour changes in this
code would be unwelcome)

-Rob

On 25/11/19 13:49, Alan Bateman wrote:
> 
> Daniel's summary is useful but changing URLClassPath doesn't feel right. Are
> you in a hurry to find a solution to this? Just asking as I think I'd prefer
> to see other options explored that fixed it in the protocol handler instead.
> 
> -Alan
> 
> On 25/11/2019 13:31, Rob McKenna wrote:
> > Thanks Daniel,
> > 
> > cc'ing core-libs-dev in case there are any objections.
> > 
> >  -Rob
> > 
> > On 25/11/19 10:47, Daniel Fuchs wrote:
> > > Hi Rob,
> > > 
> > > That looks good to me. I wonder if that should go to corelibs for
> > > review as well.
> > > 
> > > The underlying issue here is that JarURLConnection open both its
> > > jar file and its jar file entry in its connect() method.
> > > However, it delegates the opening of the jar file to the cache,
> > > when uses cache is true.
> > > In this latter case, if opening the entry throws an exception,
> > > it can't close the jar file because the file might have come from
> > > the cache and it might still be used by someone else.
> > > But because getJarFile() calls connect() then there's no way
> > > to retrieve the jar file through that (failed) connection.
> > > 
> > > I agree that fixing the issue in URLClassPath is probably the
> > > less risky.
> > > 
> > > I see that your test caters for all possible setting of uses cache
> > > and combination of success/failure when opening the jar entry,
> > > so this give me confidence that the fix is working as intended.
> > > 
> > > best regards,
> > > 
> > > -- daniel
> > > 
> > > 
> > > On 24/11/2019 15:33, Rob McKenna wrote:
> > > > Hi folks,
> > > > 
> > > > If a FileNotFoundException is thrown when attempting to load a class
> > > > from a jar file, a reference to the open JarFile remains even after the
> > > > loader is closed. The test in the webrev demonstrates the problem by
> > > > attempting to delete the JarFile after attempting a class load.
> > > > 
> > > > The deletion will fail as the JarFile is still in use (i.e. open)
> > > > despite the fact that the loader has been closed.
> > > > 
> > > > Note: the behaviour depends on the URLConnections useCaches setting so
> > > > the test cycles through the combinations. (this helpfully found a 
> > > > problem
> > > > with an earlier fix attempt)
> > > > 
> > > > bug: https://bugs.openjdk.java.net/browse/JDK-8132359
> > > > webrev: http://cr.openjdk.java.net/~robm/8132359/webrev.01/
> > > > 
> > > > Thanks,
> > > > 
> > > >   -Rob
> > > > 
> 

RFR: JDK-8277795: ldap connection timeout not honoured under contention

[Rob McKenna]

This fix attemps to resolve an issue where threads can stack up on each other 
while waiting to get a connection from the ldap pool to an unreachable server. 
It does this by having each thread start a countdown prior to holding the 
pools' lock. (which has been changed to a ReentrantLock) Once the lock has been 
grabbed, the timeout is adjusted to take the waiting time into account and the 
process of getting a connection from the pool or creating a new one commences.

Note: this fix also changes the meaning of the connection pools initSize 
somewhat. In a situation where we have a large initSize and a small timeout the 
first thread could actually exhaust the timeout before creating all of its 
initial connections. Instead this fix simply creates a single connection per 
pool-connection-request. It continues to do so for subsequent requests 
regardless of whether an existing unused connection is available in the pool 
until initSize is exhausted. As such it may require a CSR.

-

Commit messages:
 - JDK-8277795: formatting
 - JDK-8277795: whitespace cleanup
 - JDK-8277795: ldap connection timeout not honoured under contention

Changes: https://git.openjdk.java.net/jdk/pull/6568/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6568&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8277795
  Stats: 318 lines in 4 files changed: 175 ins; 72 del; 71 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6568.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6568/head:pull/6568

PR: https://git.openjdk.java.net/jdk/pull/6568

Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v2]

[Rob McKenna]

> This fix attemps to resolve an issue where threads can stack up on each other 
> while waiting to get a connection from the ldap pool to an unreachable 
> server. It does this by having each thread start a countdown prior to holding 
> the pools' lock. (which has been changed to a ReentrantLock) Once the lock 
> has been grabbed, the timeout is adjusted to take the waiting time into 
> account and the process of getting a connection from the pool or creating a 
> new one commences.
> 
> Note: this fix also changes the meaning of the connection pools initSize 
> somewhat. In a situation where we have a large initSize and a small timeout 
> the first thread could actually exhaust the timeout before creating all of 
> its initial connections. Instead this fix simply creates a single connection 
> per pool-connection-request. It continues to do so for subsequent requests 
> regardless of whether an existing unused connection is available in the pool 
> until initSize is exhausted. As such it may require a CSR.

Rob McKenna has updated the pull request incrementally with two additional 
commits since the last revision:

 - JDK-8277795: whitespace
 - Add test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6568/files
  - new: https://git.openjdk.java.net/jdk/pull/6568/files/61a7ae5b..1b679497

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6568&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6568&range=00-01

  Stats: 133 lines in 3 files changed: 131 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6568.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6568/head:pull/6568

PR: https://git.openjdk.java.net/jdk/pull/6568

Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v2]

[Rob McKenna]

On Fri, 26 Nov 2021 10:50:57 GMT, Daniel Fuchs  wrote:

> What testing is there for this fix?

I've just added a test modelled on LdapTimeoutTest.java. (with some whitespace 
issues which I'm about to fix!)

> src/java.naming/share/classes/com/sun/jndi/ldap/LdapClientFactory.java line 
> 71:
> 
>> 69: throws NamingException {
>> 70: return new LdapClient(host, port, socketFactory,
>> 71: (int)timeout, readTimeout, trace, pcb);
> 
> A blunt cast from `long` to `int` is a bit worrying as it could lead to 
> positive values becoming negative, unless you have checks in place in the 
> calling code that will ensure that the long value is never > 
> Integer.MAX_VALUE? And it could also result in a large value becoming a small 
> positive value.
> I'd suggest to remove the inconsistency one way or the other - or add an 
> explicit check to make it obvious that this case cannot happen.

Good point. I've added a check to the case. (this actually comes from 
LdapPoolManager.getLdapClient which takes an int for the connection timeout 
parameter, but it makes sense to be careful)

> src/java.naming/share/classes/com/sun/jndi/ldap/pool/Pool.java line 141:
> 
>> 139: 
>> 140: if (!conns.grabLock(remaining)) {
>> 141: throw new NamingException("Timed out waiting for lock");
> 
> Is this the appropriate exception? I see in `checkRemaining`:
> 
> throw new CommunicationException(
> "Timeout exceeded while waiting for a connection: " +
> timeout + "ms");

I've changed this to a CommuncationException.

-

PR: https://git.openjdk.java.net/jdk/pull/6568

Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v3]

[Rob McKenna]

> This fix attemps to resolve an issue where threads can stack up on each other 
> while waiting to get a connection from the ldap pool to an unreachable 
> server. It does this by having each thread start a countdown prior to holding 
> the pools' lock. (which has been changed to a ReentrantLock) Once the lock 
> has been grabbed, the timeout is adjusted to take the waiting time into 
> account and the process of getting a connection from the pool or creating a 
> new one commences.
> 
> Note: this fix also changes the meaning of the connection pools initSize 
> somewhat. In a situation where we have a large initSize and a small timeout 
> the first thread could actually exhaust the timeout before creating all of 
> its initial connections. Instead this fix simply creates a single connection 
> per pool-connection-request. It continues to do so for subsequent requests 
> regardless of whether an existing unused connection is available in the pool 
> until initSize is exhausted. As such it may require a CSR.

Rob McKenna has updated the pull request incrementally with one additional 
commit since the last revision:

  Allow the test to pass on MacOSX

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6568/files
  - new: https://git.openjdk.java.net/jdk/pull/6568/files/1b679497..7c277622

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6568&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6568&range=01-02

  Stats: 22 lines in 1 file changed: 10 ins; 1 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6568.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6568/head:pull/6568

PR: https://git.openjdk.java.net/jdk/pull/6568

Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v3]

[Rob McKenna]

On Thu, 13 Jan 2022 01:02:38 GMT, Mark Sheppard  wrote:

>> Rob McKenna has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Allow the test to pass on MacOSX
>
> src/java.naming/share/classes/com/sun/jndi/ldap/pool/PooledConnectionFactory.java
>  line 54:
> 
>> 52:  * @param timeout the connection timeout
>> 53:  */
>> 54: public abstract PooledConnection createPooledConnection(PoolCallback 
>> pcb, long timeout)
> 
> why not use int timeout to be consistent with existing code ?
> You've been required to "squash" it into an int in the factory ?

IIRC this was a request from an earlier review. (long being the standard 
throughout other new public apis) I'm happy with either, but int does avoid the 
trouble of casting.

-

PR: https://git.openjdk.java.net/jdk/pull/6568

Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v4]

[Rob McKenna]

> This fix attemps to resolve an issue where threads can stack up on each other 
> while waiting to get a connection from the ldap pool to an unreachable 
> server. It does this by having each thread start a countdown prior to holding 
> the pools' lock. (which has been changed to a ReentrantLock) Once the lock 
> has been grabbed, the timeout is adjusted to take the waiting time into 
> account and the process of getting a connection from the pool or creating a 
> new one commences.
> 
> Note: this fix also changes the meaning of the connection pools initSize 
> somewhat. In a situation where we have a large initSize and a small timeout 
> the first thread could actually exhaust the timeout before creating all of 
> its initial connections. Instead this fix simply creates a single connection 
> per pool-connection-request. It continues to do so for subsequent requests 
> regardless of whether an existing unused connection is available in the pool 
> until initSize is exhausted. As such it may require a CSR.

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

 - Add method to guard long to int cast
 - Allow the test to pass on MacOSX
 - JDK-8277795: whitespace
 - Add test
 - JDK-8277795: formatting
 - JDK-8277795: whitespace cleanup
 - JDK-8277795: ldap connection timeout not honoured under contention

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6568/files
  - new: https://git.openjdk.java.net/jdk/pull/6568/files/7c277622..6af94f37

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6568&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6568&range=02-03

  Stats: 73668 lines in 2120 files changed: 50786 ins; 12701 del; 10181 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6568.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6568/head:pull/6568

PR: https://git.openjdk.java.net/jdk/pull/6568

Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v3]

[Rob McKenna]

On Wed, 19 Jan 2022 15:53:57 GMT, Daniel Fuchs  wrote:

>> IIRC this was a request from an earlier review. (long being the standard 
>> throughout other new public apis) I'm happy with either, but int does avoid 
>> the trouble of casting.
>
> Well I guess the request was "why not use long everywhere to avoid casting to 
> int" ;-)
> But I'm happy with either too - as long as the place where you have a long 
> (e.g obtained by substracting two nano times) and call a method that takes an 
> int has the proper guards in place, and either assert/throws/floor or ceil if 
> the assumptions are not met - provided that a comment explains why that 
> particular alternative is selected.

Good point. I think its better to deal with the casts at the edges since the 
timeout handling will use long by default.

-

PR: https://git.openjdk.java.net/jdk/pull/6568

Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v3]

[Rob McKenna]

On Tue, 25 Jan 2022 15:30:46 GMT, Mark Sheppard  wrote:

>> yes a redeclaration of  timeout with a type long  across the  component 
>> would be a consistent approach, also
>
> so just to clarify, we're not taking the  approach to homogenise the timeout 
> declarations, throughout the component, to be of type long?
> 
> which would see LdapClientFactory constructor take a long timeout and  
> timeout member varaiables be redefined as long

Not in this issue. I plan to file a follow up bug to make a slight change to 
the test. I would like to get this issue fixed ASAP and would appreciate the 
time to take a good look at the transition to a long timeout. (i.e. I'll handle 
it in that follow up issue)

-

PR: https://git.openjdk.java.net/jdk/pull/6568

Integrated: JDK-8277795: ldap connection timeout not honoured under contention

[Rob McKenna]

On Thu, 25 Nov 2021 23:54:18 GMT, Rob McKenna  wrote:

> This fix attemps to resolve an issue where threads can stack up on each other 
> while waiting to get a connection from the ldap pool to an unreachable 
> server. It does this by having each thread start a countdown prior to holding 
> the pools' lock. (which has been changed to a ReentrantLock) Once the lock 
> has been grabbed, the timeout is adjusted to take the waiting time into 
> account and the process of getting a connection from the pool or creating a 
> new one commences.
> 
> Note: this fix also changes the meaning of the connection pools initSize 
> somewhat. In a situation where we have a large initSize and a small timeout 
> the first thread could actually exhaust the timeout before creating all of 
> its initial connections. Instead this fix simply creates a single connection 
> per pool-connection-request. It continues to do so for subsequent requests 
> regardless of whether an existing unused connection is available in the pool 
> until initSize is exhausted. As such it may require a CSR.

This pull request has now been integrated.

Changeset: 3d926dd6
Author:Rob McKenna 
URL:   
https://git.openjdk.java.net/jdk/commit/3d926dd66ef6551e91a4ebbbc59dcff58f5ede5a
Stats: 467 lines in 5 files changed: 324 ins; 72 del; 71 mod

8277795: ldap connection timeout not honoured under contention

Reviewed-by: dfuchs, aefimov

-

PR: https://git.openjdk.java.net/jdk/pull/6568

RFR: 8281695: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run

[Rob McKenna]

Test change to silently pass if the test environment encounters a 
NoRouteToHostException. These are intermittent at the moment but I will attempt 
to find an alternative to the use of example.com in some follow up work.

-

Commit messages:
 - 8281695: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails 
intermittently in nightly run

Changes: https://git.openjdk.java.net/jdk/pull/8925/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8925&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8281695
  Stats: 9 lines in 1 file changed: 7 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8925.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8925/head:pull/8925

PR: https://git.openjdk.java.net/jdk/pull/8925

Withdrawn: 8281695: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run

[Rob McKenna]

On Fri, 27 May 2022 15:24:46 GMT, Rob McKenna  wrote:

> Test change to silently pass if the test environment encounters a 
> NoRouteToHostException. These are intermittent at the moment but I will 
> attempt to find an alternative to the use of example.com in some follow up 
> work.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/8925

RFR: 8287672: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run

[Rob McKenna]

Test change to silently pass if the test environment encounters a 
NoRouteToHostException. These are intermittent at the moment but I will attempt 
to find an alternative to the use of example.com in some follow up work.

-

Commit messages:
 - 8281695: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails 
intermittently in nightly run

Changes: https://git.openjdk.java.net/jdk/pull/8974/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8974&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8287672
  Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8974.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8974/head:pull/8974

PR: https://git.openjdk.java.net/jdk/pull/8974

Re: Code review request: 7101658 : Backout 7082769 changes

[Rob McKenna]

Looks good.

-Rob

On 17/10/11 18:29, Alan Bateman wrote:

Seán Coffey wrote:


bug ID : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7101658
(bug report not visible yet)

changes for 7082769 fix have led to behavioral changes which can lead 
to native file descriptor exhaustion. Basically - file descriptors 
are not released until all streams referencing them are closed.


For the short term, we should backout the 7082769 changes until a 
full solution which doesn't cause issue in terms of compatibility is 
found.


I'm hoping to get this change into 7u2.

webrev : http://cr.openjdk.java.net/~coffeys/webrev.7101658/

The anti-delta looks fine.

[ As background I should explain that I asked Seán off-list to back 
out these changes because it means that closing a stream that is 
sharing a file descriptor with another stream no longer closes the 
underlying file as existing code expects. In order to fix 7082769 
properly it will likely require that FileDescriptor be changed to keep 
a reference to each of the closeables (stream and channels) that use 
it. That way when a stream is closed then it will cause all stream and 
channels using the file descriptor to be closed. This should be fixed 
in 8 first and bake for a while before considering 7u. In the 
mean-time 7u needs to be resorted to fix the current regression ].


-Alan.

code review request : JDK 7 backport: 7102369 RedirectLimit & Redirect307Test fail intermittently

[Rob McKenna]

webrev : http://cr.openjdk.java.net/~robm/7095949/webrev.00/
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7095949

-Rob

review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Hi folks,

I'm hoping for some feedback on the above. Webrev:

http://cr.openjdk.java.net/~robm/4244896/webrev.00/

This bug adds two abstract methods to Process.java with the intention of:

1) providing a facility to implement a destroyForcibly method. In 
practical terms this means provide a method that will send a SIGKILL to 
the subprocess in *nix environments as opposed to the SIGTERM that is 
currently being sent. (Windows' destroy() method effectively behaves 
like this already)


2) providing a facility to check whether the subprocess is still alive.

As per the bug report the toString/pid work has been left to be 
completed separately.


Notes:

a) It has been pointed out to me that the addition of abstract methods 
to java.lang.Process should be discussed, particularly as to whether 
default methods should be provided.


b) In src/windows/native/java/lang/ProcessImpl_md.c I chose to use 
WaitForSingleObject on the off chance that a process may choose to 
return STILL_ACTIVE as an error code.


Thanks!

-Rob

hg: jdk8/tl/jdk: 7118373: (se) Potential leak file descriptor when deregistrating at around the same time as an async close

[rob . mckenna]

Changeset: b700f85a8f29
Author:robm
Date:  2012-04-17 07:14 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b700f85a8f29

7118373: (se) Potential leak file descriptor when deregistrating at around the 
same time as an async close
Reviewed-by: alanb

! src/share/classes/sun/nio/ch/DatagramChannelImpl.java
! src/share/classes/sun/nio/ch/ServerSocketChannelImpl.java
! src/share/classes/sun/nio/ch/SocketChannelImpl.java
! src/solaris/classes/sun/nio/ch/SinkChannelImpl.java
! src/solaris/classes/sun/nio/ch/SourceChannelImpl.java

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

New webrev at:

http://cr.openjdk.java.net/~robm/4244896/webrev.01/

Differences:
- implemented Process.waitFor(long timeout). I figured I'd throw it in 
and let you decide if you want to keep it / replace isAlive.

- implemented defaults in Process.java
- destroyForcibly now returns the Process object
- Updated documentation
- Added @Override to all new overrides.
- Fixed long line in UNIXProcess_md.c consistent with other functions in 
that file.
- Replaced WaitForSingleObject with GetExitCodeProcess: given the 
concerns raised it seems to make more sense to leave users who return 
STILL_ACTIVE as an error code on their own.

- Test updated to allow for proper execution of shell script.

-Rob

On 12/04/12 10:05, Alan Bateman wrote:

On 12/04/2012 00:48, Rob McKenna wrote:

Hi folks,

I'm hoping for some feedback on the above. Webrev:

http://cr.openjdk.java.net/~robm/4244896/webrev.00/
Thanks for taking this one on, a destroyForcibly is really useful to 
have (destroy was always misleading given that that is was actually a 
SIGTERM). The isAlive is also useful, another choice would be a 
waitFor that takes a timeout.


As this patch adds two abstract methods it means there will be a 
source compatibility issue. Process has been in the platform since 
JDK1.0 so it likely there are other implementation, perhaps mocked. 
For destroyForcibly then a reasonable default could invoke the 
existing destroy but this would need to be specified. A possible 
default for isAlive is to invoke exitValue and mask the IAE when it 
hasn't terminated.


On the javadoc then destroyForcibly needs to specify the return value, 
I think I missed this in the original patch that I gave you off-list. 
Having it return the Process is very useful as it allows for method 
invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also 
think destroyForcibly needs to set expectation that the process may 
not terminate immediately (isAlive might still return true for a brief 
period for example).


The duplicate code in UNIXProcess.java.* is a reminder that we could 
combine the common code into something like a private package 
AbstractUNIXPorcess that would be extended by UNIXProcess, not for 
this patch of course.


Looks like there is inconsistent use of @Override, you've added it to 
the isAlive implementation but not the others.


Looks like UNIXProcess_md.c is straying beyond 80c so you might want 
to move the force parameter to the next line.


Overall I think the implementation looks okay but one thing to think 
about is errors, WaitForSingleObject can fail with other errors, the 
return from kill is not checked.


I don't have time to study the test is detail just now but I suspect 
invoking "./ProcessKillTest.sh" will need to change as the script will 
be in test.src and may not have execute permission. Also I see tests 
for specific exit codes which may be problematic (and will need to be 
updated for MacOSX). For isAlive then it may be better to test it by 
adding to existing tests.


-Alan.

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Eesh, thanks for spotting that Jason. New webrev will be incoming soon.

-Rob

On 18/04/12 14:44, Jason Mehrens wrote:

Rob,

It looks like waitFor is calling Object.wait(long) without owning this 
objects monitor.  If I pass Long.MAX_VALUE to waitFor, shouldn't 
waitFor return if the early if the process ends?


Jason

> Date: Tue, 17 Apr 2012 15:56:30 +0100
> From: rob.mcke...@oracle.com
> To: alan.bate...@oracle.com
> Subject: Re: review request: 4244896: (process) Provide 
System.getPid(), System.killProcess(String pid)

> CC: core-libs-dev@openjdk.java.net
>
> New webrev at:
>
> http://cr.openjdk.java.net/~robm/4244896/webrev.01/
>
> Differences:
> - implemented Process.waitFor(long timeout). I figured I'd throw it in
> and let you decide if you want to keep it / replace isAlive.
> - implemented defaults in Process.java
> - destroyForcibly now returns the Process object
> - Updated documentation
> - Added @Override to all new overrides.
> - Fixed long line in UNIXProcess_md.c consistent with other 
functions in

> that file.
> - Replaced WaitForSingleObject with GetExitCodeProcess: given the
> concerns raised it seems to make more sense to leave users who return
> STILL_ACTIVE as an error code on their own.
> - Test updated to allow for proper execution of shell script.
>
> -Rob
>
> On 12/04/12 10:05, Alan Bateman wrote:
> > On 12/04/2012 00:48, Rob McKenna wrote:
> >> Hi folks,
> >>
> >> I'm hoping for some feedback on the above. Webrev:
> >>
> >> http://cr.openjdk.java.net/~robm/4244896/webrev.00/
> > Thanks for taking this one on, a destroyForcibly is really useful to
> > have (destroy was always misleading given that that is was actually a
> > SIGTERM). The isAlive is also useful, another choice would be a
> > waitFor that takes a timeout.
> >
> > As this patch adds two abstract methods it means there will be a
> > source compatibility issue. Process has been in the platform since
> > JDK1.0 so it likely there are other implementation, perhaps mocked.
> > For destroyForcibly then a reasonable default could invoke the
> > existing destroy but this would need to be specified. A possible
> > default for isAlive is to invoke exitValue and mask the IAE when it
> > hasn't terminated.
> >
> > On the javadoc then destroyForcibly needs to specify the return 
value,

> > I think I missed this in the original patch that I gave you off-list.
> > Having it return the Process is very useful as it allows for method
> > invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also
> > think destroyForcibly needs to set expectation that the process may
> > not terminate immediately (isAlive might still return true for a 
brief

> > period for example).
> >
> > The duplicate code in UNIXProcess.java.* is a reminder that we could
> > combine the common code into something like a private package
> > AbstractUNIXPorcess that would be extended by UNIXProcess, not for
> > this patch of course.
> >
> > Looks like there is inconsistent use of @Override, you've added it to
> > the isAlive implementation but not the others.
> >
> > Looks like UNIXProcess_md.c is straying beyond 80c so you might want
> > to move the force parameter to the next line.
> >
> > Overall I think the implementation looks okay but one thing to think
> > about is errors, WaitForSingleObject can fail with other errors, the
> > return from kill is not checked.
> >
> > I don't have time to study the test is detail just now but I suspect
> > invoking "./ProcessKillTest.sh" will need to change as the script 
will

> > be in test.src and may not have execute permission. Also I see tests
> > for specific exit codes which may be problematic (and will need to be
> > updated for MacOSX). For isAlive then it may be better to test it by
> > adding to existing tests.
> >
> > -Alan.
> >
> >

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

I've uploaded another webrev to:

http://cr.openjdk.java.net/~robm/4244896/webrev.02/ 



I plan to spend some time over the coming day or two beefing up the test 
for waitFor (right now its really geared towards destroyForcibly) so I 
won't guarantee its 100% quite yet. That said, I would like feedback on 
a couple of points before I proceed:


1) Alan suggested the use of System.nanoTime() so I altered 
waitFor(long) to allow for a TimeUnit parameter. UnixProcess objects can 
use Object.wait(long, int) but unfortunately WaitForMultipleObjects (on 
Windows) only works to millisecond precision.


2) As Alan noted, there is really no need for isAlive() if people are 
happy with the idea of waitFor(long, TimeUnit). I'd appreciate any 
feedback on this aspect of the fix.


-Rob

On 19/04/12 12:05, Alan Bateman wrote:

On 19/04/2012 01:05, David Holmes wrote:

On 18/04/2012 11:44 PM, Jason Mehrens wrote:


Rob,

It looks like waitFor is calling Object.wait(long) without owning 
this objects monitor.  If I pass Long.MAX_VALUE to waitFor, 
shouldn't waitFor return if the early if the process ends?


Also waitFor doesn't call wait() under the guard of a looping 
predicate so it will suffer from lost signals and potentially 
spurious wakeups. I also don't see anything calling notify[All] to 
indicate the process has now terminated. It would appear that 
wait(timeout) is being used as a sleep mechanism and that is wrong on 
a number of levels.
I assume waitFor(timout) will require 3 distinct implementations, one 
for Solaris/Linux/Mac, another for Windows, and a default 
implementations for Process implementations that exist outside of the 
JDK.


It's likely the Solaris/Linux/Mac implementation will involve two 
threads, one to block in waitpid and the other to interrupt it via a 
signal if the timeout elapses before the child terminates. The Windows 
implementation should be trivial because it can be a timed wait.


I assume the default implementation (which is what is being discussed 
here) will need to loop calling exitValue until the timeout elapses or 
the child terminates. Not very efficient but at least it won't be used 
when when creating Processes via Runtime.exec or ProcessBuilder.


I think the question we need to consider is whether waitFor(timeout) 
is really needed. If it's something that it pushed out for another day 
then it brings up the question as to whether to include isAlive now or 
not (as waitFor without timeout gives us an isAlive equivalent too).


-Alan.

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Thanks David,

I suspect Alan simply meant that it may be preferable to use 
System.nanoTime to measure the time elapsed in the default 
implementation as opposed to System.currentTimeMillis. I had forgotten 
about the lower resolution of the timer interrupt. I'll revert that 
aspect of the change.


-Rob

On 20/04/12 02:53, David Holmes wrote:

Hi Rob,

On 20/04/2012 11:33 AM, Rob McKenna wrote:

I've uploaded another webrev to:

http://cr.openjdk.java.net/~robm/4244896/webrev.02/
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.02/>


I'll take a look as soon as I have a chance but will be OOTO the rest 
of today.



I plan to spend some time over the coming day or two beefing up the test
for waitFor (right now its really geared towards destroyForcibly) so I
won't guarantee its 100% quite yet. That said, I would like feedback on
a couple of points before I proceed:

1) Alan suggested the use of System.nanoTime() so I altered
waitFor(long) to allow for a TimeUnit parameter. UnixProcess objects can
use Object.wait(long, int) but unfortunately WaitForMultipleObjects (on
Windows) only works to millisecond precision.


Object.wait (like Thread.sleep) doesn't honour the nanosecond argument 
so there is no point trying to use it.


David
-


2) As Alan noted, there is really no need for isAlive() if people are
happy with the idea of waitFor(long, TimeUnit). I'd appreciate any
feedback on this aspect of the fix.

-Rob

On 19/04/12 12:05, Alan Bateman wrote:

On 19/04/2012 01:05, David Holmes wrote:

On 18/04/2012 11:44 PM, Jason Mehrens wrote:


Rob,

It looks like waitFor is calling Object.wait(long) without owning
this objects monitor. If I pass Long.MAX_VALUE to waitFor, shouldn't
waitFor return if the early if the process ends?


Also waitFor doesn't call wait() under the guard of a looping
predicate so it will suffer from lost signals and potentially
spurious wakeups. I also don't see anything calling notify[All] to
indicate the process has now terminated. It would appear that
wait(timeout) is being used as a sleep mechanism and that is wrong on
a number of levels.

I assume waitFor(timout) will require 3 distinct implementations, one
for Solaris/Linux/Mac, another for Windows, and a default
implementations for Process implementations that exist outside of the
JDK.

It's likely the Solaris/Linux/Mac implementation will involve two
threads, one to block in waitpid and the other to interrupt it via a
signal if the timeout elapses before the child terminates. The Windows
implementation should be trivial because it can be a timed wait.

I assume the default implementation (which is what is being discussed
here) will need to loop calling exitValue until the timeout elapses or
the child terminates. Not very efficient but at least it won't be used
when when creating Processes via Runtime.exec or ProcessBuilder.

I think the question we need to consider is whether waitFor(timeout)
is really needed. If it's something that it pushed out for another day
then it brings up the question as to whether to include isAlive now or
not (as waitFor without timeout gives us an isAlive equivalent too).

-Alan.

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Thanks a lot for the feedback folks.

Jason, as Alan notes, that makes a lot of sense. Thanks for that.

I'm planning to do the following over the next couple of days:

- fix UnixProcess.waitFor(long, TimeUnit) as per David's mail.
- alter the default waitFor(long, TimeUnit) comments/implementation as 
per Alan's comments.

- put some more work into firming up the test.

I'll get back to the alias with a new webrev when I'm done.

One thing I would note is that there is a "process reaper" thread in the 
UnixProcess implementations that will call Process.notifyAll() when the 
process exits.


-Rob

On 20/04/12 17:09, Alan Bateman wrote:

On 20/04/2012 02:33, Rob McKenna wrote:

I've uploaded another webrev to:

http://cr.openjdk.java.net/~robm/4244896/webrev.02/ 
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.02/>


I plan to spend some time over the coming day or two beefing up the 
test for waitFor (right now its really geared towards 
destroyForcibly) so I won't guarantee its 100% quite yet. That said, 
I would like feedback on a couple of points before I proceed:


1) Alan suggested the use of System.nanoTime() so I altered 
waitFor(long) to allow for a TimeUnit parameter. UnixProcess objects 
can use Object.wait(long, int) but unfortunately 
WaitForMultipleObjects (on Windows) only works to millisecond precision.


2) As Alan noted, there is really no need for isAlive() if people are 
happy with the idea of waitFor(long, TimeUnit). I'd appreciate any 
feedback on this aspect of the fix.


-Rob

Sorry I haven't time to reply before now.

I think destroyForcibly and isAlive are quite good. In destroyForcibly 
then "invokes destroy" should probably be a link or @code. The @since 
should be after the @return.  For isAlive then I would suggest "Tests 
whether the subprocess is live" rather an "Returns a boolean ...".


waitFor needs discussion. In 1 above you mentioned nanoTime but I 
think I was actually suggesting we need to decide if the method is 
old-style waitFor(timeout) or j.u.c/new-style 
waitFor(timeout,TimeUnit). As Process doesn't have timeouts already 
then I don't see a problem introducing the j.u.c style. I see you've 
also changed it to return a boolean too so overall the signature looks 
much better. I did question whether we need both isAlive and 
waitFor(0L,...) but Jason's comment make sense, you don't want to have 
exception handling to poll a sub-process. On the javadoc then I don't 
think it should mention that it polls every 100ms, that's too 
closterfobic and someone will likely want to test it too.


On the implementation then it looks like Windows is right but the 
Solaris/Linux/Mac implementation doesn't wakeup if the sub-process 
terminates before the timeout has elapsed. If this proves too 
difficult then going forward with the destroyForcibly + isAlive, 
leaving waitFor to another day is fine. On the default implementation 
(pre-jdk8 Process implementations outside of the JDK) then I assume 
this method will misbehave with a large timeout (the addition will 
overflow). You could probably adjust the sleep timeout when the 
remaining time is <100ms.


-Alan.

hg: jdk8/tl/jdk: 7166687: InetAddress.getLocalHost().getHostName() returns FQDN

[rob . mckenna]

Changeset: b26c04717735
Author:robm
Date:  2012-05-07 13:34 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b26c04717735

7166687: InetAddress.getLocalHost().getHostName() returns FQDN
Reviewed-by: chegar

! src/solaris/native/java/net/Inet6AddressImpl.c

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Hi folks,

The latest version is at:

http://cr.openjdk.java.net/~robm/4244896/webrev.03/ 



Feedback greatly appreciated.

-Rob

On 19/04/12 12:05, Alan Bateman wrote:

On 19/04/2012 01:05, David Holmes wrote:

On 18/04/2012 11:44 PM, Jason Mehrens wrote:


Rob,

It looks like waitFor is calling Object.wait(long) without owning 
this objects monitor.  If I pass Long.MAX_VALUE to waitFor, 
shouldn't waitFor return if the early if the process ends?


Also waitFor doesn't call wait() under the guard of a looping 
predicate so it will suffer from lost signals and potentially 
spurious wakeups. I also don't see anything calling notify[All] to 
indicate the process has now terminated. It would appear that 
wait(timeout) is being used as a sleep mechanism and that is wrong on 
a number of levels.
I assume waitFor(timout) will require 3 distinct implementations, one 
for Solaris/Linux/Mac, another for Windows, and a default 
implementations for Process implementations that exist outside of the 
JDK.


It's likely the Solaris/Linux/Mac implementation will involve two 
threads, one to block in waitpid and the other to interrupt it via a 
signal if the timeout elapses before the child terminates. The Windows 
implementation should be trivial because it can be a timed wait.


I assume the default implementation (which is what is being discussed 
here) will need to loop calling exitValue until the timeout elapses or 
the child terminates. Not very efficient but at least it won't be used 
when when creating Processes via Runtime.exec or ProcessBuilder.


I think the question we need to consider is whether waitFor(timeout) 
is really needed. If it's something that it pushed out for another day 
then it brings up the question as to whether to include isAlive now or 
not (as waitFor without timeout gives us an isAlive equivalent too).


-Alan.

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Hi Paul,

Comments inline:

On 11/05/12 12:29, Paul Sandoz wrote:

Hi Rob,

I dunno if the following has been pointed out before. It's hard to track review 
comments.


The implementation of Process.waitFor normalizes the end time and duration 
remaining time to nano seconds.  However, the Thread.sleep method expects a 
duration in units of milliseconds.

You could use:

   http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#sleep(long, 
int)

Or just round things up to the nearest millisecond:

   Thread.sleep(Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));

That would be more consistent and wait for less time over that of the requested 
duration.

Thanks for catching that.


For the following code:

  227 long rem = end - now;
  228 while (!hasExited&&  (rem>  0)) {
  229 wait(TimeUnit.NANOSECONDS.toMillis(rem));
  230 rem = end - System.nanoTime();
  231 }

Can the above go into a spin loop once the duration is less than 1 millisecond 
for the rest of that duration? If so you may want to round it up to the nearest 
millisecond.
Eesh. wait(0) will wait until notified so we could end up waiting past 
our timeout expiry. Would:


wait(Math.max(TimeUnit.NANOSECONDS.toMillis(rem), 1));

alleviate all concerns here?

-Rob

Paul.

On May 10, 2012, at 8:56 PM, Rob McKenna wrote:


Hi folks,

The latest version is at:

http://cr.openjdk.java.net/~robm/4244896/webrev.03/<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>

Feedback greatly appreciated.

-Rob

On 19/04/12 12:05, Alan Bateman wrote:

On 19/04/2012 01:05, David Holmes wrote:

On 18/04/2012 11:44 PM, Jason Mehrens wrote:

Rob,

It looks like waitFor is calling Object.wait(long) without owning this objects 
monitor.  If I pass Long.MAX_VALUE to waitFor, shouldn't waitFor return if the 
early if the process ends?

Also waitFor doesn't call wait() under the guard of a looping predicate so it 
will suffer from lost signals and potentially spurious wakeups. I also don't 
see anything calling notify[All] to indicate the process has now terminated. It 
would appear that wait(timeout) is being used as a sleep mechanism and that is 
wrong on a number of levels.

I assume waitFor(timout) will require 3 distinct implementations, one for 
Solaris/Linux/Mac, another for Windows, and a default implementations for 
Process implementations that exist outside of the JDK.

It's likely the Solaris/Linux/Mac implementation will involve two threads, one 
to block in waitpid and the other to interrupt it via a signal if the timeout 
elapses before the child terminates. The Windows implementation should be 
trivial because it can be a timed wait.

I assume the default implementation (which is what is being discussed here) 
will need to loop calling exitValue until the timeout elapses or the child 
terminates. Not very efficient but at least it won't be used when when creating 
Processes via Runtime.exec or ProcessBuilder.

I think the question we need to consider is whether waitFor(timeout) is really 
needed. If it's something that it pushed out for another day then it brings up 
the question as to whether to include isAlive now or not (as waitFor without 
timeout gives us an isAlive equivalent too).

-Alan.

hg: jdk8/tl/jdk: 7168110: Misleading jstack error message

[rob . mckenna]

Changeset: 178c480998b1
Author:robm
Date:  2012-05-17 22:42 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/178c480998b1

7168110: Misleading jstack error message
Reviewed-by: alanb, dsamersoff

! src/windows/native/sun/tools/attach/WindowsVirtualMachine.c

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Latest version including work on the spec language:

http://cr.openjdk.java.net/~robm/4244896/webrev.04/ 
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>


-Rob

On 10/05/12 19:56, Rob McKenna wrote:

Hi folks,

The latest version is at:

http://cr.openjdk.java.net/~robm/4244896/webrev.03/ 
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>


Feedback greatly appreciated.

-Rob

On 19/04/12 12:05, Alan Bateman wrote:

On 19/04/2012 01:05, David Holmes wrote:

On 18/04/2012 11:44 PM, Jason Mehrens wrote:


Rob,

It looks like waitFor is calling Object.wait(long) without owning 
this objects monitor.  If I pass Long.MAX_VALUE to waitFor, 
shouldn't waitFor return if the early if the process ends?


Also waitFor doesn't call wait() under the guard of a looping 
predicate so it will suffer from lost signals and potentially 
spurious wakeups. I also don't see anything calling notify[All] to 
indicate the process has now terminated. It would appear that 
wait(timeout) is being used as a sleep mechanism and that is wrong 
on a number of levels.
I assume waitFor(timout) will require 3 distinct implementations, one 
for Solaris/Linux/Mac, another for Windows, and a default 
implementations for Process implementations that exist outside of the 
JDK.


It's likely the Solaris/Linux/Mac implementation will involve two 
threads, one to block in waitpid and the other to interrupt it via a 
signal if the timeout elapses before the child terminates. The 
Windows implementation should be trivial because it can be a timed wait.


I assume the default implementation (which is what is being discussed 
here) will need to loop calling exitValue until the timeout elapses 
or the child terminates. Not very efficient but at least it won't be 
used when when creating Processes via Runtime.exec or ProcessBuilder.


I think the question we need to consider is whether waitFor(timeout) 
is really needed. If it's something that it pushed out for another 
day then it brings up the question as to whether to include isAlive 
now or not (as waitFor without timeout gives us an isAlive equivalent 
too).


-Alan.

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

That link should be:

http://cr.openjdk.java.net/~robm/4244896/webrev.04/

-Rob

On 31/05/12 16:05, Rob McKenna wrote:

Latest version including work on the spec language:

http://cr.openjdk.java.net/~robm/4244896/webrev.04/ 
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>


-Rob

On 10/05/12 19:56, Rob McKenna wrote:

Hi folks,

The latest version is at:

http://cr.openjdk.java.net/~robm/4244896/webrev.03/ 
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>


Feedback greatly appreciated.

-Rob

On 19/04/12 12:05, Alan Bateman wrote:

On 19/04/2012 01:05, David Holmes wrote:

On 18/04/2012 11:44 PM, Jason Mehrens wrote:


Rob,

It looks like waitFor is calling Object.wait(long) without owning 
this objects monitor.  If I pass Long.MAX_VALUE to waitFor, 
shouldn't waitFor return if the early if the process ends?


Also waitFor doesn't call wait() under the guard of a looping 
predicate so it will suffer from lost signals and potentially 
spurious wakeups. I also don't see anything calling notify[All] to 
indicate the process has now terminated. It would appear that 
wait(timeout) is being used as a sleep mechanism and that is wrong 
on a number of levels.
I assume waitFor(timout) will require 3 distinct implementations, 
one for Solaris/Linux/Mac, another for Windows, and a default 
implementations for Process implementations that exist outside of 
the JDK.


It's likely the Solaris/Linux/Mac implementation will involve two 
threads, one to block in waitpid and the other to interrupt it via a 
signal if the timeout elapses before the child terminates. The 
Windows implementation should be trivial because it can be a timed 
wait.


I assume the default implementation (which is what is being 
discussed here) will need to loop calling exitValue until the 
timeout elapses or the child terminates. Not very efficient but at 
least it won't be used when when creating Processes via Runtime.exec 
or ProcessBuilder.


I think the question we need to consider is whether waitFor(timeout) 
is really needed. If it's something that it pushed out for another 
day then it brings up the question as to whether to include isAlive 
now or not (as waitFor without timeout gives us an isAlive 
equivalent too).


-Alan.

hg: jdk8/tl/jdk: 7161881: (dc) DatagramChannel.bind(null) fails if IPv4 socket and running with preferIPv6Addresses=true

[rob . mckenna]

Changeset: a7895dc61088
Author:robm
Date:  2012-06-08 18:23 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a7895dc61088

7161881: (dc) DatagramChannel.bind(null) fails if IPv4 socket and running with 
preferIPv6Addresses=true
Reviewed-by: alanb, chegar

! src/share/classes/sun/nio/ch/DatagramChannelImpl.java
+ test/java/nio/channels/DatagramChannel/BindNull.java

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Hi folks,

5th, and hopefully final review has been posted to:

http://cr.openjdk.java.net/~robm/4244896/webrev.05/ 
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.05/>


Let me know if there are any comments or concerns, and thanks a lot for 
the help so far.


-Rob

On 01/06/12 01:41, David Holmes wrote:

Hi Rob,

This looks good to me. I'm glad to see that destroyForcibly mandates 
that Process instances from ProcessBuilder.start and Runtime.exec must 
do a forcible destroy. That addresses my concern about documenting the 
actual implementations.


Two minor comments:

Process.java:

 236  * {@link ProcessBuilder#start} and {@link Runtime#exec} are 
of type that


"are of type" -> "are of a type ..."

ProcessKillTest.sh:

BIT_FLAG is now unused.

Thanks,
David


On 1/06/2012 1:05 AM, Rob McKenna wrote:

Latest version including work on the spec language:

http://cr.openjdk.java.net/~robm/4244896/webrev.04/
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>

-Rob

On 10/05/12 19:56, Rob McKenna wrote:

Hi folks,

The latest version is at:

http://cr.openjdk.java.net/~robm/4244896/webrev.03/
<http://cr.openjdk.java.net/%7Erobm/4244896/webrev.03/>

Feedback greatly appreciated.

-Rob

On 19/04/12 12:05, Alan Bateman wrote:

On 19/04/2012 01:05, David Holmes wrote:

On 18/04/2012 11:44 PM, Jason Mehrens wrote:


Rob,

It looks like waitFor is calling Object.wait(long) without owning
this objects monitor. If I pass Long.MAX_VALUE to waitFor,
shouldn't waitFor return if the early if the process ends?


Also waitFor doesn't call wait() under the guard of a looping
predicate so it will suffer from lost signals and potentially
spurious wakeups. I also don't see anything calling notify[All] to
indicate the process has now terminated. It would appear that
wait(timeout) is being used as a sleep mechanism and that is wrong
on a number of levels.

I assume waitFor(timout) will require 3 distinct implementations, one
for Solaris/Linux/Mac, another for Windows, and a default
implementations for Process implementations that exist outside of the
JDK.

It's likely the Solaris/Linux/Mac implementation will involve two
threads, one to block in waitpid and the other to interrupt it via a
signal if the timeout elapses before the child terminates. The
Windows implementation should be trivial because it can be a timed 
wait.


I assume the default implementation (which is what is being discussed
here) will need to loop calling exitValue until the timeout elapses
or the child terminates. Not very efficient but at least it won't be
used when when creating Processes via Runtime.exec or ProcessBuilder.

I think the question we need to consider is whether waitFor(timeout)
is really needed. If it's something that it pushed out for another
day then it brings up the question as to whether to include isAlive
now or not (as waitFor without timeout gives us an isAlive equivalent
too).

-Alan.

hg: jdk8/tl/jdk: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[rob . mckenna]

Changeset: ff0da4ea08a2
Author:robm
Date:  2012-06-26 13:27 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ff0da4ea08a2

4244896: (process) Provide System.getPid(), System.killProcess(String pid)
Reviewed-by: alanb

! src/share/classes/java/lang/Process.java
! src/solaris/classes/java/lang/UNIXProcess.java.bsd
! src/solaris/classes/java/lang/UNIXProcess.java.linux
! src/solaris/classes/java/lang/UNIXProcess.java.solaris
! src/solaris/native/java/lang/UNIXProcess_md.c
! src/windows/classes/java/lang/ProcessImpl.java
! src/windows/native/java/lang/ProcessImpl_md.c
! test/java/lang/ProcessBuilder/Basic.java
+ test/java/lang/ProcessBuilder/DestroyTest.java

Re: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[Rob McKenna]

Thanks Thomas,

As per Alan's mail, we're going to deal with this as a separate issue 
after we've discussed it.


-Rob

On 25/06/12 10:52, Alan Bateman wrote:

On 25/06/2012 09:56, Thomas Stüfe wrote:

Hi, Rob,

src/solaris/native/java/lang/UNIXProcess_md.c:

You never check the return code of kill(2). kill() may fail if you
have no permission to kill the process (EPERM), or if the process id
is invalid, maybe it has already been reaped. In both cases an
exception would be nice, or if you decide this cannot happen in the
context of Process.java, at least an assert().

One thing to add to this is that the destroy method has been there 
since jdk1.0 and is not defined or specified to throw any exceptions 
so we can't really change that now. From the history it doesn't look 
like the return from kill(2) has ever been checked. As it can only ne 
used to send a signal to processes created by the ProcessBuilder or 
Runtime.exec API (not arbitrary process) then it is unlikely to fail. 
Whether the new destroy method should specify an exception may require 
consideration and maybe a separate bug should be created to consider 
that.


-Alan

hg: jdk8/tl/jdk: 7174887: Deadlock in jndi ldap connection cleanup

[rob . mckenna]

Changeset: ecc5dd3790a1
Author:robm
Date:  2012-07-02 19:32 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ecc5dd3790a1

7174887: Deadlock in jndi ldap connection cleanup
Reviewed-by: xuelei

! src/share/classes/com/sun/jndi/ldap/Connection.java
! src/share/classes/com/sun/jndi/ldap/LdapClient.java

Request for review: 7179305: (fs) Method name sun.nio.fs.UnixPath.getPathForExecptionMessage is misspelled

[Rob McKenna]

http://cr.openjdk.java.net/~robm/7179305/webrev.01/ 



Thanks,

-Rob

hg: jdk8/tl/jdk: 7179305: (fs) Method name sun.nio.fs.UnixPath.getPathForExecptionMessage is misspelled

[rob . mckenna]

Changeset: 516e0c884af2
Author:robm
Date:  2012-07-09 22:26 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/516e0c884af2

7179305: (fs) Method name sun.nio.fs.UnixPath.getPathForExecptionMessage is 
misspelled
Reviewed-by: dholmes, chegar

! src/solaris/classes/sun/nio/fs/LinuxUserDefinedFileAttributeView.java
! src/solaris/classes/sun/nio/fs/LinuxWatchService.java
! src/solaris/classes/sun/nio/fs/SolarisAclFileAttributeView.java
! src/solaris/classes/sun/nio/fs/SolarisUserDefinedFileAttributeView.java
! src/solaris/classes/sun/nio/fs/SolarisWatchService.java
! src/solaris/classes/sun/nio/fs/UnixCopyFile.java
! src/solaris/classes/sun/nio/fs/UnixException.java
! src/solaris/classes/sun/nio/fs/UnixFileSystemProvider.java
! src/solaris/classes/sun/nio/fs/UnixPath.java

Codereview request: 6931128: (spec) File attribute tests fail when run as root.

[Rob McKenna]

Hi folks,

Looking for a codereview for a spec change in jdk8.

In summary, when a java application is run as root it can perform file 
operations regardless of the permissions attributed to that file. I.e. 
canExecute, canRead, canWrite would return true even though the File may 
not have those permissions set.


webrev at: http://cr.openjdk.java.net/~robm/6931128/webrev.01/ 



Thanks,

-Rob

hg: jdk8/tl/jdk: 6931128: (spec) File attribute tests fail when run as root.

[rob . mckenna]

Changeset: da14e2c90bcb
Author:robm
Date:  2012-08-15 22:46 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/da14e2c90bcb

6931128: (spec) File attribute tests fail when run as root.
Reviewed-by: alanb

! src/share/classes/java/io/File.java
! test/java/io/File/Basic.java
! test/java/io/File/SetAccess.java
! test/java/io/File/SetReadOnly.java
! test/java/io/File/SymLinks.java
+ test/java/io/File/Util.java

Request for review: 7191777: test/java/lang/ProcessBuilder/Basic.java failing intermittently due to additions for 4244896

[Rob McKenna]

Hi folks,

One of the tests from 4244896 failed once during nightly testing. It 
isn't known how much of a delay will be necessary in order for it to 
pass. In any case the tolerance can't really be loosened much more 
without making the test meaningless so I've decided to remove it.


http://cr.openjdk.java.net/~robm/7191777/webrev.01/ 



Thanks,

-Rob

Re: Request for review: 7191777: test/java/lang/ProcessBuilder/Basic.java failing intermittently due to additions for 4244896

[Rob McKenna]

Sounds good:

http://cr.openjdk.java.net/~robm/7191777/webrev.02/test/java/lang/ProcessBuilder/Basic.java.cdiff.html 
<http://cr.openjdk.java.net/%7Erobm/7191777/webrev.02/test/java/lang/ProcessBuilder/Basic.java.cdiff.html>


-Rob

On 16/08/12 08:19, Alan Bateman wrote:

On 16/08/2012 03:18, David Holmes wrote:

Hi Rob,

On 16/08/2012 9:09 AM, Rob McKenna wrote:

Hi folks,

One of the tests from 4244896 failed once during nightly testing. It
isn't known how much of a delay will be necessary in order for it to
pass. In any case the tolerance can't really be loosened much more
without making the test meaningless so I've decided to remove it.

http://cr.openjdk.java.net/~robm/7191777/webrev.01/
<http://cr.openjdk.java.net/%7Erobm/7191777/webrev.01/>


Can we not leave the waitFor in place but simply not check how long 
we waited? That way if it really takes "too long" we hit the default 
test timeout.
That seems a good idea as it also exercises waitFor at around the time 
that the process is terminating.


-Alan.

hg: jdk8/tl/jdk: 7191777: test/java/lang/ProcessBuilder/Basic.java failing intermittently due to additions for 4244896

[rob . mckenna]

Changeset: 59aa7660ade4
Author:robm
Date:  2012-08-20 14:52 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/59aa7660ade4

7191777: test/java/lang/ProcessBuilder/Basic.java failing intermittently due to 
additions for 4244896
Reviewed-by: dholmes, alanb

! test/java/lang/ProcessBuilder/Basic.java

hg: jdk8/tl/jdk: 7199862: Make sure that a connection is still alive when retrieved from KeepAliveCache in certain cases

[rob . mckenna]

Changeset: 11a5da68673c
Author:robm
Date:  2012-09-27 22:35 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/11a5da68673c

7199862: Make sure that a connection is still alive when retrieved from 
KeepAliveCache in certain cases
Reviewed-by: chegar

! src/share/classes/sun/net/www/http/HttpClient.java
! src/share/classes/sun/net/www/protocol/http/HttpURLConnection.java

hg: jdk8/tl/jdk: 7199219: Proxy-Connection headers set incorrectly when a HttpClient is retrieved from the Keep Alive Cache

[rob . mckenna]

Changeset: b3c7a3138c5d
Author:robm
Date:  2012-09-28 04:39 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b3c7a3138c5d

7199219: Proxy-Connection headers set incorrectly when a HttpClient is 
retrieved from the Keep Alive Cache
Reviewed-by: chegar

! src/share/classes/sun/net/www/http/HttpClient.java
! src/share/classes/sun/net/www/protocol/http/HttpURLConnection.java

RFR: 7152183: TEST_BUG: java/lang/ProcessBuilder/Basic.java failing intermittently [sol]

[Rob McKenna]

Hi folks,

The only way I can see this test failing in this manner[*] is if we 
destroy the process before we begin the read. That being the case I've 
jacked up the sleep (giving the reader thread a little more time to get 
cracking) and added a check to see if the threads stack has entered a 
read call.


http://cr.openjdk.java.net/~robm/7152183/webrev.01/ 



Feedback greatly appreciated.

-Rob


[*] le trace:

java.io.IOException: Stream Closed
at java.io.FileInputStream.read(Native Method)
at java.lang.UNIXProcess$DeferredCloseInputStream.read(UNIXProcess.java:253)
at Basic$64.run(Basic.java:1908)
java.lang.AssertionError: Some tests failed
at Basic.main(Basic.java:2248)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

at java.lang.reflect.Method.invoke(Method.java:474)
at com.sun.javatest.regtest.MainWrapper$MainThread.run(MainWrapper.java:94)
at java.lang.Thread.run(Thread.java:722)

for 
http://hg.openjdk.java.net/jdk8/tl/jdk/file/d45bc4307996/test/java/lang/ProcessBuilder/Basic.java


(current version differs in line number)

hg: jdk8/tl/jdk: 7184932: Remove the temporary Selector usage in the NIO socket adapters

[rob . mckenna]

Changeset: bba370caafad
Author:robm
Date:  2012-10-04 19:53 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bba370caafad

7184932: Remove the temporary Selector usage in the NIO socket adapters
Reviewed-by: alanb

! make/java/nio/mapfile-bsd
! make/java/nio/mapfile-linux
! make/java/nio/mapfile-solaris
! src/share/classes/sun/nio/ch/DatagramChannelImpl.java
! src/share/classes/sun/nio/ch/DatagramSocketAdaptor.java
! src/share/classes/sun/nio/ch/Net.java
! src/share/classes/sun/nio/ch/ServerSocketAdaptor.java
! src/share/classes/sun/nio/ch/ServerSocketChannelImpl.java
! src/share/classes/sun/nio/ch/SocketAdaptor.java
! src/share/classes/sun/nio/ch/SocketChannelImpl.java
! src/share/classes/sun/nio/ch/Util.java
! src/solaris/native/sun/nio/ch/Net.c
! src/windows/native/sun/nio/ch/Net.c
+ test/java/nio/channels/etc/AdaptorCloseAndInterrupt.java

Re: RFR: 7152183: TEST_BUG: java/lang/ProcessBuilder/Basic.java failing intermittently [sol]

[Rob McKenna]

Thanks Martin,

I've uploaded a new webrev to:

http://cr.openjdk.java.net/~robm/7152183/webrev.02/ 
<http://cr.openjdk.java.net/%7Erobm/7152183/webrev.02/>


Let me know if this does the job.

-Rob

On 04/10/12 18:24, Martin Buchholz wrote:

Hi all,

Yeah, this particular test is rather racy - sorry about that.
We need to call p.destroy when the other thread is in the middle of a 
read() system call, and there's no way to know for sure - seeing java 
"read" in the stacktrace is not enough, since it may not have gotten 
to the system call yet.


suggestions:
pull the computation of the inputstream before the latch to narrow the 
window a bit:


final InputStream s;
switch (action & 0x1) {
case 0: s = p.getInputStream(); break;
case 1: s = p.getErrorStream(); break;
default: throw
}
latch.countdown();
switch (action & 0x2) {
case 0: r = s.read(); break;
case 1: r = s.read(bytes); break;
}

Examining the stack trace to look for "read" is clever but does not 
actually eliminate the race.


Looking in UNIXProcess.java.solaris I see the use 
of DeferredCloseInputStream.  We can eliminate the race on solaris 
(i.e. if the inputstream.getclass isDeferredCloseInputStream)  by 
looping until the useCount field of the DeferredCloseInputStream is > 
0, using ugly but effective reflective code.  This should allow us to 
avoid the horrible sleep for 200ms.


You should use yield instead of sleep between loop iterations while 
waiting for the useCount to be bumped.


On other platforms this is not an issue, I think.

Martin

On Thu, Oct 4, 2012 at 12:39 AM, Alan Bateman <mailto:alan.bate...@oracle.com>> wrote:


On 03/10/2012 22:44, Rob McKenna wrote:

Hi folks,

The only way I can see this test failing in this manner[*] is
if we destroy the process before we begin the read. That being
the case I've jacked up the sleep (giving the reader thread a
little more time to get cracking) and added a check to see if
the threads stack has entered a read call.

http://cr.openjdk.java.net/~robm/7152183/webrev.01/
<http://cr.openjdk.java.net/%7Erobm/7152183/webrev.01/>
<http://cr.openjdk.java.net/%7Erobm/7152183/webrev.01/>

Feedback greatly appreciated.

-Rob


[*] le trace:

So stack traces are masculine, I didn't know that.

I think your analysis is right, it's just that the sleep(10) is
not sufficient to ensure that the thread gets to the read method.
Increasing the sleep is probably sufficient. The hack to look at
the stack trace makes it more robust for really extreme cases, at
the cost of potential further maintenance in the event that the
implementation changes. In any case it's good to resolve this
intermittent test failure.

-Alan

Re: RFR: 7152183: TEST_BUG: java/lang/ProcessBuilder/Basic.java failing intermittently [sol]

[Rob McKenna]

Thanks Martin,

Updated webrev at:

http://cr.openjdk.java.net/~robm/7152183/webrev.03/ 
<http://cr.openjdk.java.net/%7Erobm/7152183/webrev.03/>


and a few comments inline:

On 08/10/12 18:52, Martin Buchholz wrote:

Thanks for the changes - this approach looks good.  But:

1954 case 0: r = s.read(bytes); break;
1955 case 2: r = s.read(bytes); break;

The two cases are the same code; you want

case 0: r = s.read(); break;

Eesh, typo, sorry.


1964 String os = System.getProperty("os.name 
<http://os.name>");

1965 if (os.equalsIgnoreCase("Solaris") ||
1966 os.equalsIgnoreCase("SunOS"))

I wouldn't bother with testing for Solaris explicitly, and just rely 
on reflective code testing the implementation of s.
Loop for useCount to go non-negative If and only if we find it 
reflectively.


1970 if (s.toString().startsWith(
1971 "java.lang.UNIXProcess$DeferredCloseInputStream"))

It's bad style to depend on the output of toString() - better is 
something like


Class c = s.getClass();
if (c.getName().equals(...)) {

Oh, now I see the difficulty - the DeferredCloseInputStream may or may 
not be wrapped in a BufferedInputStream.

Which makes the reflective code much more annoying.
Yes, that being the case I'm going to leave in the OS check, is that ok? 
Also yes, c.getName() makes far more sense, thanks.


1976 useCount = (Integer)useCountField.get(s);

I think you want to use getInt (not get) on a field of type int.

Yes I do.


1987 while (useCount.intValue() <= 0) {
1988 useCount = (Integer)useCountField.get(s);
1989 Thread.currentThread().yield();
1990 }

I was imagining a loop that looks more like this:

if (useCountField != null) {
while (useCountField.getInt(s) <= 0) {
  Thread.currentThread().yield();
}
}
Thats much nicer. I was thinking I'd leave the null check out as if 
useCountField is null at this point, the test should probably fail. Let 
me know if thats ok.


I'm surprised you're not seeing IAE when calling useCountField.get(s) 
when wrapped in a BIS.  Shouldn't that be a call 
to useCountField.get(deferred) instead?  Can we fix this in the 
wrapped case by assigning "s = deferred"?


Sorry, that was a mistake. Rectified now. I hadn't actually exercised 
that code due to my while loop implementation and the fact that the 
problem wasn't reproducing. Thanks for spotting that.


-Rob

Thanks,

Martin

On Sun, Oct 7, 2012 at 12:48 PM, Rob McKenna <mailto:rob.mcke...@oracle.com>> wrote:


Thanks Martin,

I've uploaded a new webrev to:

http://cr.openjdk.java.net/~robm/7152183/webrev.02/
<http://cr.openjdk.java.net/%7Erobm/7152183/webrev.02/>

Let me know if this does the job.


-Rob

On 04/10/12 18:24, Martin Buchholz wrote:

Hi all,

Yeah, this particular test is rather racy - sorry about that.
We need to call p.destroy when the other thread is in the middle
of a read() system call, and there's no way to know for sure -
seeing java "read" in the stacktrace is not enough, since it may
not have gotten to the system call yet.

suggestions:
pull the computation of the inputstream before the latch to
narrow the window a bit:

final InputStream s;
switch (action & 0x1) {
case 0: s = p.getInputStream(); break;
case 1: s = p.getErrorStream(); break;
default: throw
}
latch.countdown();
switch (action & 0x2) {
case 0: r = s.read(); break;
case 1: r = s.read(bytes); break;
}

Examining the stack trace to look for "read" is clever but does
not actually eliminate the race.

Looking in UNIXProcess.java.solaris I see the use
of DeferredCloseInputStream.  We can eliminate the race on
solaris (i.e. if the inputstream.getclass
isDeferredCloseInputStream)  by looping until the useCount field
of the DeferredCloseInputStream is > 0, using ugly but effective
reflective code.  This should allow us to avoid the horrible
sleep for 200ms.

You should use yield instead of sleep between loop iterations
while waiting for the useCount to be bumped.

On other platforms this is not an issue, I think.

Martin

On Thu, Oct 4, 2012 at 12:39 AM, Alan Bateman
mailto:alan.bate...@oracle.com>> wrote:

On 03/10/2012 22:44, Rob McKenna wrote:

Hi folks,

The only way I can see this test failing in this
manner[*] is if we destroy the process before we begin
the read. That being the case I've jacked up the sleep
(giving the reader thread a little more time to get
cracking) and added a che

hg: jdk8/tl/jdk: 7152183: TEST_BUG: java/lang/ProcessBuilder/Basic.java failing intermittently [sol]

[rob . mckenna]

Changeset: 7c2f5e52863c
Author:robm
Date:  2012-10-11 18:24 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7c2f5e52863c

7152183: TEST_BUG: java/lang/ProcessBuilder/Basic.java failing intermittently 
[sol]
Reviewed-by: alanb, martin, dholmes

! test/java/lang/ProcessBuilder/Basic.java

Request for review: 8000487: Java JNDI connection library on ldap conn is not honoring configured timeout

[Rob McKenna]

Hi folks,

Seemingly when using ldap's simple authentication we perform a 
readReply. (an operation which is subject to
com.sun.jndi.ldap.read.timeout as opposed to 
com.sun.jndi.ldap.connect.timeout) This makes an apparent connection to 
a faulty host appear to take much longer than the specified connect 
timeout. The proposed solution is to set the read timeout value to the 
same value as the connect for the duration of the authentication call.


In addition to this I've merged the testcase for this issue with the 
testcases for other ldap timeout issues.


http://cr.openjdk.java.net/~robm/8000487/webrev.01/ 



Thanks,

-Rob

RFR: 8000817: Reinstate accidentally removed sleep() from ProcessBuilder/Basic.java

[Rob McKenna]

Hi folks,

Managed to remove the Thread.sleep(10) from the test when fixing 
7152183. Sorry about that.


http://cr.openjdk.java.net/~robm/8000817/webrev.01/ 



-Rob

Re: RFR: 8000817: Reinstate accidentally removed sleep() from ProcessBuilder/Basic.java

[Rob McKenna]

Sorry for not responding sooner, I was out for the evening.

I threw this fix into our test infrastructure (jprt) before I left 
though, and I see it has passed.


http://cr.openjdk.java.net/~robm/8000817/webrev.02/ 



I'm a little unclear as to why you'd prefer to throw that away, can you 
elaborate?


As ugly as a Thread.sleep(10) is, it hasn't historically been a problem 
on platforms other than Solaris. Perhaps this in combination with the 
stack hackery?


Let me know either way and I'll get it integrated sharpish! Thanks,

-Rob


On 12/10/12 19:54, Martin Buchholz wrote:



On Fri, Oct 12, 2012 at 11:47 AM, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:



Checking for readBytes should be better but it might still be
fragile because there isn't any guarantee that the thread is in
the read syscall (although the window should be a lot smaller).
Although none of us like using sleep, this may be a case where we
need a short sleep to improve our chances.


Yup.

Sure would be nice if we could see native methods in the stacktraces, 
kind of like this:


   java.lang.Thread.State: RUNNABLE
at (C/C++) __kernel_vsyscall( ())
at (C/C++) readBytes( (jre/lib/i386/libjava.so))
at (C/C++) Java_java_io_FileInputStream_readBytes( 
(jre/lib/i386/libjava.so))

at java.io.FileInputStream.readBytes(Native Method)

Re: RFR: 8000817: Reinstate accidentally removed sleep() from ProcessBuilder/Basic.java

[Rob McKenna]

The test passes. But as far as the race goes that may be a fluke. I'll 
add the sleep to be on the safe side. Thanks Martin,


-Rob

On 12/10/12 23:20, Martin Buchholz wrote:

Thanks, Rob.

I'm OK with your webrev.02, and I'm OK with putting back a 10ms sleep, 
if you want to also do that.  I'm not sure what happens on macosx or 
windows - you might want to think about that.


Martin

On Fri, Oct 12, 2012 at 1:18 PM, Rob McKenna <mailto:rob.mcke...@oracle.com>> wrote:


Sorry for not responding sooner, I was out for the evening.

I threw this fix into our test infrastructure (jprt) before I left
though, and I see it has passed.

http://cr.openjdk.java.net/~robm/8000817/webrev.02/
<http://cr.openjdk.java.net/%7Erobm/8000817/webrev.02/>

I'm a little unclear as to why you'd prefer to throw that away,
can you elaborate?

As ugly as a Thread.sleep(10) is, it hasn't historically been a
problem on platforms other than Solaris. Perhaps this in
combination with the stack hackery?

Let me know either way and I'll get it integrated sharpish! Thanks,

-Rob



On 12/10/12 19:54, Martin Buchholz wrote:



On Fri, Oct 12, 2012 at 11:47 AM, Alan Bateman
mailto:alan.bate...@oracle.com>> wrote:


Checking for readBytes should be better but it might still be
fragile because there isn't any guarantee that the thread is
in the read syscall (although the window should be a lot
smaller). Although none of us like using sleep, this may be a
case where we need a short sleep to improve our chances.


Yup.

Sure would be nice if we could see native methods in the
stacktraces, kind of like this:

   java.lang.Thread.State: RUNNABLE
at (C/C++) __kernel_vsyscall( ())
at (C/C++) readBytes( (jre/lib/i386/libjava.so))
at (C/C++) Java_java_io_FileInputStream_readBytes(
(jre/lib/i386/libjava.so))
at java.io.FileInputStream.readBytes(Native Method)

hg: jdk8/tl/jdk: 8000487: Java JNDI connection library on ldap conn is not honoring configured timeout

[rob . mckenna]

Changeset: c0736b62160e
Author:robm
Date:  2012-10-15 22:34 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c0736b62160e

8000487: Java JNDI connection library on ldap conn is not honoring configured 
timeout
Reviewed-by: vinnie

! src/share/classes/com/sun/jndi/ldap/Connection.java
! src/share/classes/com/sun/jndi/ldap/LdapClient.java
+ test/com/sun/jndi/ldap/LdapTimeoutTest.java
- test/com/sun/jndi/ldap/LdapsReadTimeoutTest.java
- test/com/sun/jndi/ldap/ReadTimeoutTest.java

RFR: 8000975: Merge UNIXProcess.java.bsd & UNIXProcess.java.linux

[Rob McKenna]

It has been suggested to me (at least twice at this stage) that these 
files should be merged since they're identical. This requires an 
adjustment in perspective from "every platform should have its own 
implementation" to "those platforms that differ from the norm should 
have their own implementation". Given that the latter approach 
simplifies things currently I believe it is the more sensible.


Webrev at:

http://cr.openjdk.java.net/~robm/8000975/webrev.01/ 



-Rob

Re: RFR: 8000975: Merge UNIXProcess.java.bsd & UNIXProcess.java.linux

[Rob McKenna]

That is true, and I'm just working on bringing Mike's Solaris changes 
forward now. I hadn't considered that it would result in a difference in 
Mac too. I'll get this work completed and see where it stands first.


-Rob

On 16/10/12 17:35, Alan Bateman wrote:

On 16/10/2012 17:15, Rob McKenna wrote:
It has been suggested to me (at least twice at this stage) that these 
files should be merged since they're identical. This requires an 
adjustment in perspective from "every platform should have its own 
implementation" to "those platforms that differ from the norm should 
have their own implementation". Given that the latter approach 
simplifies things currently I believe it is the more sensible.


Webrev at:

http://cr.openjdk.java.net/~robm/8000975/webrev.01/ 
<http://cr.openjdk.java.net/%7Erobm/8000975/webrev.01/>


-Rob
I don't have any objection to this but at some point I think we need 
to change the Solaris and Mac implementations to use posix_spawn, in 
which case it's possible that the Mac implementation will be different 
to the Linux implementation.


-Alan

hg: jdk7/tl/corba: 2 new changesets

[rob . mckenna]

Changeset: f642c9ec81a0
Author:robm
Date:  2010-11-15 10:46 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/corba/rev/f642c9ec81a0

6277781: Serialization of Enums over IIOP is broke.
Summary: Reviewed by Ken Cavanaugh
Reviewed-by: coffeys

! src/share/classes/com/sun/corba/se/impl/io/IIOPInputStream.java

Changeset: cff5a173ec1e
Author:robm
Date:  2010-11-15 10:47 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/corba/rev/cff5a173ec1e

6763340: memory leak in com.sun.corba.se.* classes
6873605: Missing finishedDispatch() call in ORBImpl causes test failures after 
5u20 b04
Summary: Reviewed by Ken Cavanaugh
Reviewed-by: coffeys

! 
src/share/classes/com/sun/corba/se/impl/interceptors/ClientRequestInfoImpl.java
! src/share/classes/com/sun/corba/se/impl/interceptors/PIHandlerImpl.java
! src/share/classes/com/sun/corba/se/impl/interceptors/PINoOpHandlerImpl.java
! src/share/classes/com/sun/corba/se/impl/interceptors/RequestInfoImpl.java
! src/share/classes/com/sun/corba/se/impl/orb/ORBImpl.java
! 
src/share/classes/com/sun/corba/se/impl/protocol/CorbaClientRequestDispatcherImpl.java
! src/share/classes/com/sun/corba/se/spi/protocol/PIHandler.java
+ src/share/classes/com/sun/corba/se/spi/protocol/RetryType.java

RFR: 8129957 - Deadlock in JNDI LDAP implementation when closing the LDAP context

[Rob McKenna]

Hi folks,

We have a hang between LdapClient / Ctx due to the fact that 
Connection.cleanup() calls LdapClient.processConnectionClosure which 
locks the unsolicited vector and in turn calls LdapCtx.fireUnsolicited 
which locks the eventSupport object. Unfortunately when an 
LdapCtx.close() occurs at the same time, its removeUnsolicited method 
locks the eventSupport object first and then attempts to call 
LdapClient.removeUnsolicited which attempts to lock the unsolicited vector.


So:

thread 1:

Connection.cleanup ->
LdapClient.processConnectionClosure (LOCK VECTOR) ->
LdapCtx.fireUnsolicited (LOCK EVENTSUPPORT)

(LdapClient is looping through LdapCtx objects in the unsolicited vector)

thread 2:

LdapCtx.close (LOCK LDAPCTX) ->
LdapCtx.removeUnsolicited (LOCK EVENTSUPPORT) ->
LdapClient.removeUnsolicited (LOCK VECTOR)

(A single LdapCtx removes itself from its LdapClient unsolicited list)


My proposed solution is to have both threads lock the LdapClient before 
locking either the unsolicited vector or the eventSupport object.


Webrev at: http://cr.openjdk.java.net/~robm/8129957/webrev.01/

-Rob

Re: RFR/RFA (8u-dev) 8133253: [TESTBUG] java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java fails on compact profile

[Rob McKenna]

Approved pending positive codrereview.

-Rob

On 19/08/15 15:57, Ivan Gerasimov wrote:

Hi!

The test FieldSetAccessibleTest.java fails when testing with compact3
even after fixing JDK-8080524.
The reason is that the test tries to access classes which are only
available in full jdk.

The proposed fix is to include the test in the corresponding group in
TEST.groups file.
Would you please help review the fix and approve pushing it into
jdk8u-dev repo?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8133253
WEBREV: http://cr.openjdk.java.net/~igerasim/8133253/00/webrev/

Sincerely yours,
Ivan

Re: RFR: 8129957 - Deadlock in JNDI LDAP implementation when closing the LDAP context

[Rob McKenna]

Hi folks,

So on further investigation it looks like we could get away with 
reducing the amount of locking in LdapClient. Here is a proposed fix 
followed by a description:


http://cr.openjdk.java.net/~robm/8129957/webrev.02/

processConnectionClosure():
- Remove the synchronization from processConnectionClosure and handle it 
further down in notifyUnsolicited


removeUnsolicited():
- Remove the synchronized block in removeUnsolicited as its redundant. 
Vectors are synchronized already.


processUnsolicited()
- Remove the initial synchronized block in processUnsolicited and limit 
it to the area around the unsolicited size check / notice creation. 
(again, due to the notifyUnsolicited changes)
- Remove the redundant unsolicited.size check from the end of 
processUnsolicited


notifyUnsolicited():
- Synchronize on the unsolicited vector in order to create a copy of 
that vector and empty it if e is a NamingException.
- Outside the notifyUnsolicited synchronize block, loop through the copy 
of unsolicited and call fireUnsolicited on each element.
- The main potential problem with this fix would be if an LdapCtx became 
unsolicited before we got to it in the for loop. However since both 
LdapCtx.fireUnsolicited and LdapCtx.removeUnsolicited sync on 
eventSupport and LdapCtx.fireUnsolicited double checks to make sure it 
is still unsolicited, that should be fine.


-Rob


On 10/08/15 14:06, Rob McKenna wrote:

Hi folks,

We have a hang between LdapClient / Ctx due to the fact that
Connection.cleanup() calls LdapClient.processConnectionClosure which
locks the unsolicited vector and in turn calls LdapCtx.fireUnsolicited
which locks the eventSupport object. Unfortunately when an
LdapCtx.close() occurs at the same time, its removeUnsolicited method
locks the eventSupport object first and then attempts to call
LdapClient.removeUnsolicited which attempts to lock the unsolicited vector.

So:

thread 1:

Connection.cleanup ->
LdapClient.processConnectionClosure (LOCK VECTOR) ->
LdapCtx.fireUnsolicited (LOCK EVENTSUPPORT)

(LdapClient is looping through LdapCtx objects in the unsolicited vector)

thread 2:

LdapCtx.close (LOCK LDAPCTX) ->
LdapCtx.removeUnsolicited (LOCK EVENTSUPPORT) ->
LdapClient.removeUnsolicited (LOCK VECTOR)

(A single LdapCtx removes itself from its LdapClient unsolicited list)


My proposed solution is to have both threads lock the LdapClient before
locking either the unsolicited vector or the eventSupport object.

Webrev at: http://cr.openjdk.java.net/~robm/8129957/webrev.01/

 -Rob

RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString

[Rob McKenna]

Hi folks,

Requesting a review of this change which switches corelibs usages of the 
thread-unsafe strerror over to strerror_r/strerror_s:


http://cr.openjdk.java.net/~robm/8133249/webrev.01/

-Rob

Re: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString

[Rob McKenna]

Thanks Christos:

On 21/09/15 17:32, chris...@zoulas.com wrote:

On Sep 21,  4:53pm, rob.mcke...@oracle.com (Rob McKenna) wrote:
-- Subject: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr

| Hi folks,
|
| Requesting a review of this change which switches corelibs usages of the
| thread-unsafe strerror over to strerror_r/strerror_s:
|
| http://cr.openjdk.java.net/~robm/8133249/webrev.01/

I like this change, but in jdk_strerror.c:

1. Why are you using "errno" instead of "err" in the linux strerror_r()
invocation?


Gah. An oversight. Thanks for catching it.


2. Isn't the "Unknown error" test going to break on non-english locales?


Unless I'm misreading the gnu implementation appears to hardcode 
"Unknown error" string for strerror_r. (but not strerror_l)


I'll have a new webrev up soon that addresses the first comment and 
Rogers feedback.


-Rob



Best,

christos

Re: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString

[Rob McKenna]

Hi folks,

Uploaded a newer version:

http://cr.openjdk.java.net/~robm/8133249/webrev.02/

To address Rogers question, this needed to be separate from 
getLastErrorString because of the return type and the err arg, but I 
like the idea of avoiding the creation of a new file. Also, all of the 
files that require the new function (now called getErrorString) already 
include jni_util.c so it makes sense to put this in jni_util_md.c with 
getLastErrorString.


As per Christos' suggestion I've mapped strerr_r to __xpg_strerror_r 
instead of using the gnu strerror_r. I'd be interested to hear feedback 
on this change.


I've corrected the oversights around the jli sources and 
PlainDatagramSocket. The jli stuff shouldn't have been there in the 
first place.


Ivan, I've kept the return type. As you note, I should have been using 
it in ProcessImpl_md.c and the extra information from the return is 
potentially useful.


-Rob

On 23/09/15 14:14, Ivan Gerasimov wrote:

Hi Rob!

On 21.09.2015 18:53, Rob McKenna wrote:

Hi folks,

Requesting a review of this change which switches corelibs usages of
the thread-unsafe strerror over to strerror_r/strerror_s:

http://cr.openjdk.java.net/~robm/8133249/webrev.01/



I think it would be better to have  jdk_strerror(errno, tmpbuf,
sizeof(tmpbuf))  instead of  jdk_strerror(errno, tmpbuf, (size_t) 1024),
just for the sake of avoiding constant duplication.
I don't see the return code of jdk_strerror() is ever used. I suggest
using it in ProcessImpl_md.c (see below), or just making this function
return void.

jni_util_md.c:
- why can't we just call  jdk_strerror(errno, buf, len)  and get rid of
the temp buffer?

java_md_common.c:
- commented out #include @ line 26
- the change in JLI_ReportErrorMessageSys() looks incomplete

ProcessImpl_md.c:
- would it make sense to check if jdk_strerror() == EINVAL instead of
comparing the strings?

PlainDatagramSocketImpl.c:
- #include added, but no other changes were made.

TwoStacksPlainDatagramSocketImpl.c:
- buf is declared to be char[1024], but only up to 255 chars are used.
- would be better to move the buf declaration closer to its use, maybe
next to char errmsg[300] line?

jdk_strerror.c:
- minor nit: extra space after strerror_r at lines 46 and 63.

Sincerely yours,
Ivan

RFR: 8135124 - com/sun/jndi/ldap/LdapTimeoutTest.java failed intermittently

[Rob McKenna]

Hi folks,

I'd like to comment the ReadTimeoutTest part of this test for now as it 
appears to be causing intermittent failures. (on the understanding that 
these failures will be investigated separately)


http://cr.openjdk.java.net/~robm/8135124/webrev.01/

-Rob

Re: RFR - 8133249: Occasional SIGSEGV: non thread-safe use of strerr in getLastErrorString

[Rob McKenna]

Thanks Ivan, I'll push with these changes.

-Rob

On 24/09/15 16:31, Ivan Gerasimov wrote:

Thanks Rob!

The webrev looks cleaner.

jni_util.h:
Wouldn't it be better to move this block directly to the jni_util_md.c
file?
  390 #if defined(LINUX) && (defined(_GNU_SOURCE) || \
  391  (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE < 200112L \
  392  && defined(_XOPEN_SOURCE) && _XOPEN_SOURCE < 600))
  393 extern int __xpg_strerror_r(int, char *, size_t);
  394 #endif
I don't see the __xpg_strerror_r() function used anywhere outside that
.c file anyway.


TwoStacksPlainDatagramSocketImpl.c:
It wasn't introduced by your fix, but it seems that a new-line isn't
necessary here:
2218 sprintf(errmsg, "error getting socket option: %s\n", tmpbuf);

In all other places JNU_ThrowByName() uses non-new-line-terminated
messages.

Sincerely yours,
Ivan

On 24.09.2015 17:10, Rob McKenna wrote:

Hi folks,

Uploaded a newer version:

http://cr.openjdk.java.net/~robm/8133249/webrev.02/

To address Rogers question, this needed to be separate from
getLastErrorString because of the return type and the err arg, but I
like the idea of avoiding the creation of a new file. Also, all of the
files that require the new function (now called getErrorString)
already include jni_util.c so it makes sense to put this in
jni_util_md.c with getLastErrorString.

As per Christos' suggestion I've mapped strerr_r to __xpg_strerror_r
instead of using the gnu strerror_r. I'd be interested to hear
feedback on this change.

I've corrected the oversights around the jli sources and
PlainDatagramSocket. The jli stuff shouldn't have been there in the
first place.

Ivan, I've kept the return type. As you note, I should have been using
it in ProcessImpl_md.c and the extra information from the return is
potentially useful.

-Rob

On 23/09/15 14:14, Ivan Gerasimov wrote:

Hi Rob!

On 21.09.2015 18:53, Rob McKenna wrote:

Hi folks,

Requesting a review of this change which switches corelibs usages of
the thread-unsafe strerror over to strerror_r/strerror_s:

http://cr.openjdk.java.net/~robm/8133249/webrev.01/



I think it would be better to have  jdk_strerror(errno, tmpbuf,
sizeof(tmpbuf))  instead of  jdk_strerror(errno, tmpbuf, (size_t) 1024),
just for the sake of avoiding constant duplication.
I don't see the return code of jdk_strerror() is ever used. I suggest
using it in ProcessImpl_md.c (see below), or just making this function
return void.

jni_util_md.c:
- why can't we just call  jdk_strerror(errno, buf, len)  and get rid of
the temp buffer?

java_md_common.c:
- commented out #include @ line 26
- the change in JLI_ReportErrorMessageSys() looks incomplete

ProcessImpl_md.c:
- would it make sense to check if jdk_strerror() == EINVAL instead of
comparing the strings?

PlainDatagramSocketImpl.c:
- #include added, but no other changes were made.

TwoStacksPlainDatagramSocketImpl.c:
- buf is declared to be char[1024], but only up to 255 chars are used.
- would be better to move the buf declaration closer to its use, maybe
next to char errmsg[300] line?

jdk_strerror.c:
- minor nit: extra space after strerror_r at lines 46 and 63.

Sincerely yours,
Ivan

Re: [8u-dev] Request for review and approval: 8139863: [TESTBUG] Need to port tests for JDK-8134903 to 8u-dev

[Rob McKenna]

Sorry for the delay Michael, I had understood the subject to mean you 
were waiting on another explicit review.


Approved.

-Rob

On 04/11/15 08:39, Michael Haupt wrote:

All,

the code in question has been reviewed already: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-February/031871.html

Best,

Michael


Am 02.11.2015 um 09:12 schrieb Michael Haupt :

(Apologies for multiple posts with wrong subject lines.)


Dear all,

cross-posted to jdk8u-dev and core-libs-dev.

Please review this change.
Issue: https://bugs.openjdk.java.net/browse/JDK-8139863 

Webrev: http://cr.openjdk.java.net/~mhaupt/8139863/webrev.00 


This adds a missing test to 8u-dev that was not pushed along with the feature 
backport it belongs to. The backport was approved; see 
http://mail.openjdk.java.net/pipermail/jdk8u-dev/2015-September/004185.html 
.

Thanks,

Michael

RFR: 8132455 - com/sun/jndi/ldap/LdapTimeoutTest.java fails at handleNamingException

[Rob McKenna]

So unfortunately this isn't a real fix for this failing test. 
(intermittent, certain platforms only) However since I have enough 
information to investigate this problem separately I'd like to silence 
this specific failure in the mean time. I'll investigate the actual 
failure under https://bugs.openjdk.java.net/browse/JDK-8141370


http://cr.openjdk.java.net/~robm/8132455/webrev.01/

-Rob

RFR: 8140344: add support for 3 digit update release numbers

[Rob McKenna]

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing 
infrastructure balks at the extra digit. With that in mind I'd like to 
update the logic surrounding the parsing of version numbers and the test 
itself to use regex instead of the less flexible CharacterSequence method.


http://cr.openjdk.java.net/~robm/8140344/webrev.01/

-Rob

Re: RFR: 8140344: add support for 3 digit update release numbers

[Rob McKenna]

I'd expect this to be superseded by 223 completely but I've cc'd 
verona-dev in case there are objections. At this point the fix is more 
for the benefit of 7 & 8. I'd be happy to mark this 9-na if that makes 
more sense?


-Rob

On 17/11/15 15:19, Alan Bateman wrote:



On 17/11/2015 14:47, Rob McKenna wrote:

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing
infrastructure balks at the extra digit. With that in mind I'd like to
update the logic surrounding the parsing of version numbers and the
test itself to use regex instead of the less flexible
CharacterSequence method.

http://cr.openjdk.java.net/~robm/8140344/webrev.01/

Would it be better to bring this to verona-dev so that this can be
looked at in the context of the JEP 223 changes?

-Alan

Re: RFR: 8140344: add support for 3 digit update release numbers

[Rob McKenna]

Thanks David, I'll mark this 9-na. (bcc'ing verona-dev)

Something similar was fixed in 7 
(https://bugs.openjdk.java.net/browse/JDK-8009634) - that may be what 
you're thinking of.


Aside from that are you ok with the changes?

-Rob

On 18/11/15 09:00, David Holmes wrote:

On 18/11/2015 1:30 AM, Rob McKenna wrote:

I'd expect this to be superseded by 223 completely but I've cc'd
verona-dev in case there are objections. At this point the fix is more
for the benefit of 7 & 8. I'd be happy to mark this 9-na if that makes
more sense?


I'd vote for a 7 (and 8?) specific fix. Didn't this already get fixed in 6.

David


 -Rob

On 17/11/15 15:19, Alan Bateman wrote:



On 17/11/2015 14:47, Rob McKenna wrote:

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing
infrastructure balks at the extra digit. With that in mind I'd like to
update the logic surrounding the parsing of version numbers and the
test itself to use regex instead of the less flexible
CharacterSequence method.

http://cr.openjdk.java.net/~robm/8140344/webrev.01/

Would it be better to bring this to verona-dev so that this can be
looked at in the context of the JEP 223 changes?

-Alan

Re: RFR: 8140344: add support for 3 digit update release numbers

[Rob McKenna]

You would think, but this fix isn't in jdk_util.c in 6. The test isn't 
in 6 either though.


-Rob

On 24/11/15 04:39, David Holmes wrote:

On 24/11/2015 12:24 AM, Rob McKenna wrote:

Thanks David, I'll mark this 9-na. (bcc'ing verona-dev)

Something similar was fixed in 7
(https://bugs.openjdk.java.net/browse/JDK-8009634) - that may be what
you're thinking of.


6 hit this with 6u101 first, so presumably it was addressed there (and
there was a big bug tail for that).


Aside from that are you ok with the changes?


Sorry I didn't actually review the code.

David


 -Rob

On 18/11/15 09:00, David Holmes wrote:

On 18/11/2015 1:30 AM, Rob McKenna wrote:

I'd expect this to be superseded by 223 completely but I've cc'd
verona-dev in case there are objections. At this point the fix is more
for the benefit of 7 & 8. I'd be happy to mark this 9-na if that makes
more sense?


I'd vote for a 7 (and 8?) specific fix. Didn't this already get fixed
in 6.

David


 -Rob

On 17/11/15 15:19, Alan Bateman wrote:



On 17/11/2015 14:47, Rob McKenna wrote:

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing
infrastructure balks at the extra digit. With that in mind I'd
like to
update the logic surrounding the parsing of version numbers and the
test itself to use regex instead of the less flexible
CharacterSequence method.

http://cr.openjdk.java.net/~robm/8140344/webrev.01/

Would it be better to bring this to verona-dev so that this can be
looked at in the context of the JEP 223 changes?

-Alan

8141370: com/sun/jndi/ldap/LdapTimeoutTest.java failed intermittently

[Rob McKenna]

Hi folks,

Hopefully this fix puts this tests' failures to bed. (mind you I've 
thought that before)


Basically I've separated the failing subtest out into its own file and 
excluded it on the (intermittently) failing platforms.


http://cr.openjdk.java.net/~robm/8141370/webrev.01/

-Rob

Re: 8141370: com/sun/jndi/ldap/LdapTimeoutTest.java failed intermittently

[Rob McKenna]

Thanks, just spotted that myself after attempting to run it in a loop 
via jtreg. (in case jtreg is somehow involved in causing the 
intermittent failure) Will be corrected pre-push.


-Rob

On 08/12/15 15:45, Daniel Fuchs wrote:

Hey Rob,

The @run line seems to be wrong in the new test file:

  26  * @run main/othervm LdapTimeoutTest

cheers,

-- daniel

On 08/12/15 15:00, Rob McKenna wrote:

Hi folks,

Hopefully this fix puts this tests' failures to bed. (mind you I've
thought that before)

Basically I've separated the failing subtest out into its own file and
excluded it on the (intermittently) failing platforms.

http://cr.openjdk.java.net/~robm/8141370/webrev.01/

 -Rob

Re: RFR(s): 8148425: strerror() function is not thread-safe

[Rob McKenna]

On 27/02/16 11:20, David Holmes wrote:
> On 27/02/2016 5:00 AM, Volker Simonis wrote:
> >Hi Thomas,
> >
> >it's good that somebody finally addresses this long standing issue :)
> >
> >However I wonder if it would be possible to align this effort with the
> >core libraries group (CC'ed). They already fix this issue with:
> >
> >8133249: Occasional SIGSEGV: non thread-safe use of strerr in 
> >getLastErrorString
> >https://bugs.openjdk.java.net/browse/JDK-8133249
> >
> >I would be nice if we could use the same version in hotspot and the
> >core libraries.
> 
> I don't find this:
> 
> +#if defined(LINUX) && (defined(_GNU_SOURCE) || \
> + (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE < 200112L \
> + && defined(_XOPEN_SOURCE) && _XOPEN_SOURCE < 600))
> +extern int __xpg_strerror_r(int, char *, size_t);
> +#define strerror_r(a, b, c) __xpg_strerror_r((a), (b), (c))
> +#endif
> 
> particularly appealing at all - you can't even decode it without having a
> POSIX and XOpen version history in front of you :(
> 
> And a version that requires a buffer to be passed in is problematic in a
> number of usage contexts in hotspot - see the comments in the bug report. A
> simplified version that converts symbolic error values to their string
> equivalent is much more appealing - albeit fixing an issue that "should" be
> handled at the library level.

Agreed, its horrific. Unfortunately any fix for strerror thread safety is 
effectively a kludge on top of an ugly library situation. This fix seems to be 
going down the route of reimplementing sys_errlist (which iirc Solaris has 
removed and Linux has deprecated) I didn't feel confident taking that path with 
my fix but this may be a case of what works for hotspot may be different to 
what works for corelibs.

-Rob

> 
> David
> -
> 
> >Regards,
> >Volker
> >
> >
> >On Fri, Feb 26, 2016 at 5:05 PM, Thomas Stüfe  
> >wrote:
> >>Hi,
> >>
> >>please take a look at this proposed fix:
> >>
> >>Bug: https://bugs.openjdk.java.net/browse/JDK-8148425
> >>Webrev:
> >>http://cr.openjdk.java.net/~stuefe/webrevs/8141425-strerror-replacement/webrev.00/webrev/
> >>
> >>This adds a replacement function os::strerror() as a drop-in for
> >>strerror(), which has a number of issues.
> >>
> >>strerror() is unsafe to use and may cause (and has caused) crashes in
> >>multithreaded scenarios. It is also not ideal for log files because of the
> >>implicit localization of the error messages.
> >>
> >>For details please see the discussion under the bug report.
> >>
> >>Please note that I did not yet change any call sites, although all call
> >>sites in the os namespace should already use the new function. I wanted to
> >>see whether there would be any general objections.
> >>
> >>Kind Regards, Thomas

Re: RFR 9 8043477: java/lang/ProcessBuilder/Basic.java failed with: java.lang.AssertionError: Some tests failed

[Rob McKenna]

Eesh, Sorry Roger, I have something like this on my todo.

Martin, my concern with the long delay approach is that it effectively 
nullifies the point of the test. Given that this test has been flakey 
the approach has been to simply bump the acceptable delta by another 
100ms or so every time. Since we've had to do this repeatedly however, 
I'm beginning to question whether the test is more trouble than its worth.


-Rob

On 12/11/14 19:44, Martin Buchholz wrote:

The print statement below seems redundant with the assertion failure message.
You could improve the assertion message instead if need be.
Adding thousand separator underscores to 2L would help a little.

I like my little helper

 static long millisElapsedSince(long startNanoTime) {
 return NANOSECONDS.toMillis(System.nanoTime() - startNanoTime);
 }


+System.out.printf(" waitFor process: delta: %d%n",(end - start) );
+
  if ((end - start) > 2L * (AIX.is() ? 2 : 1))
  fail("Test failed: waitFor took too long (" + (end -
start) + "ns)");

200 ms timeout for subprocesses to finish is just too damn low.  In
j.u.c. tests we switched to 10 seconds for most "long" timeouts
(LONG_DELAY_MS) and are happy with the disappearance of rare flaky
results.


On Wed, Nov 12, 2014 at 11:34 AM, roger riggs  wrote:

Please review test changes to ProcessBuilder Basic.java to add some
debugging information.
The test has been failing intermittently.  The wait times have been extended
to see
if the systems are just slow.  The failure criteria have not changed.

Suggestions welcome.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-debug-8043477/

issue:
8043477: java/lang/ProcessBuilder/Basic.java failed with:
java.lang.AssertionError: Some tests failed

Thanks, Roger

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

RFR: 8065238 - javax.naming.NamingException after upgrade to JDK 8

[Rob McKenna]

Hi folks,

Looking to fix a regression caused by 8042857. Basically the behaviour 
in 8042857 is incorrect. This fix reverts to the previous behaviour and 
attempts to beef up the tests a little around Ldap timeouts.


http://cr.openjdk.java.net/~robm/8065238/webrev.01/

The test itself looks quite complex but isn't really. There are two 
executor pools. (scheduled and fixed) The fixed pool is for running 
tests concurrently. The scheduled pool is for killing tests that test 
ldap connects / reads where no timeout is set. (according to the spec 
these should wait forever)


For these long running timeout tests, we schedule a thread to interrupt 
the test after waiting for 20s. There are 3 of these long running tests 
in total, hence the decision to run the tests in parallel.


I'm not averse to breaking this out into separate tests and a library 
for the helper classes if people think it makes more sense to leave the 
concurrency up to the test framework.


-Rob

Re: RFR: 8065238 - javax.naming.NamingException after upgrade to JDK 8

[Rob McKenna]

Just updated the test to workaround a seemingly unrelated platform 
specific issue. (that only manifests on older kernels)


http://cr.openjdk.java.net/~robm/8065238/webrev.02/

-Rob

On 03/12/14 16:21, Rob McKenna wrote:

Hi folks,

Looking to fix a regression caused by 8042857. Basically the behaviour 
in 8042857 is incorrect. This fix reverts to the previous behaviour 
and attempts to beef up the tests a little around Ldap timeouts.


http://cr.openjdk.java.net/~robm/8065238/webrev.01/

The test itself looks quite complex but isn't really. There are two 
executor pools. (scheduled and fixed) The fixed pool is for running 
tests concurrently. The scheduled pool is for killing tests that test 
ldap connects / reads where no timeout is set. (according to the spec 
these should wait forever)


For these long running timeout tests, we schedule a thread to 
interrupt the test after waiting for 20s. There are 3 of these long 
running tests in total, hence the decision to run the tests in parallel.


I'm not averse to breaking this out into separate tests and a library 
for the helper classes if people think it makes more sense to leave 
the concurrency up to the test framework.


-Rob

Re: 5049299 - (process) Use,posix_spawn, not fork, on S10 to avoid swap,exhaustion (jdk7u-dev)

[Rob McKenna]

Hi Amit,

As per https://bugs.openjdk.java.net/browse/JDK-5049299, this is fixed 
in 7u55+ (including 8 fcs). There are some peculiarities however:


On JDK8 posix_spawn is the default on Solaris & Mac. vfork is the 
default on Linux.
On JDK7u55+ posix_spawn is the default on Mac only. fork is the default 
on Solaris and vfork is the default on Linux.


On 7u55+ you can change the default launch mechanism using the 
jdk.lang.Process.launchMechanism startup flag. E.g.:


java -Djdk.lang.Process.launchMechanism=posix_spawn ...

-Rob


On 29/12/14 14:51, Amit Baxi wrote:

Hi Folks,

We are facing issue with process builder on JRockit JVM with solaris 
5.10 machine. That is realted to following bug:


   http://bugs.java.com/view_bug.do?bug_id=5049299



Currently we are using JRockit JDK 1.6.0_37-R28.2.5-4.1.0 version. 
Please let me know  following :


 * Which version of JRockit this bug is fixed ? (as it is fixed in JAVA
   8u 15)
 * If yes then  is it also fixed for solaris 5.10 or In solaris process
   builder still uses fork() call ?


As per this page link below Mr. Rob wrote that for solaris it still 
using fork which is confusing weather fixed for solaris or not:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-November/022909.html 



Please confirm for this fix and thanks in advance.

Waiting for your response,

Thanks!
*Amit Baxi



*

RFR: 8077822: javac does not recognize '*.java' as file if '-J' option is specified

[Rob McKenna]

Hi folks,

Because of platform specifics the Java launcher contains some extra 
wildcard expansion processing on Windows.


As part of this processing the list of args received by 
CreateApplicationArgs (java_md.c) is compared to the original list in 
the java launchers main method.


Unfortunately the CreateApplicationArgs list has already been filtered 
by TranslateApplicationArgs which removes VM specific flags. This 
results in the launcher incorrectly neglecting to expand wildcard arguments.


This fix filters the main method args in the same way so 
CreateApplicationArgs will be comparing like with like.


http://cr.openjdk.java.net/~robm/8077822/webrev.01/

-Rob

Re: RFR: 8077822: javac does not recognize '*.java' as file if '-J' option is specified

[Rob McKenna]

Thanks Kumar, I'll alter (& build / test) it before pushing.

-Rob

On 19/05/15 19:22, Kumar Srinivasan wrote:

Hi Rob,

Looks good, except a small nit, I would've written this with a positive
check

+/* Copy the non-vm args */
+for (i = 0; i < nargc ; i++) {
+const char *arg = stdargs[i].arg;
+if (arg[0] != '-' || arg[1] != 'J') {
+argv = (StdArg*) JLI_MemRealloc(argv, (nargs+1) *
sizeof(StdArg));
+argv[nargs].arg = JLI_StringDup(arg);
+argv[nargs].has_wildcard = stdargs[i].has_wildcard;
+nargs++;
+}
+}


as follows.

 /* Copy the non-vm args */
 for (i = 0; i < nargc ; i++) {
 const char *arg = stdargs[i].arg;
 if (arg[0] == '-' && arg[1] == 'J') // OR if (JLI_StrNCmp(arg,
"-J", JLI_StrLen("-J") == 0)
 continue;
 argv = (StdArg*) JLI_MemRealloc(argv, (nargs+1) * sizeof(StdArg));
 argv[nargs].arg = JLI_StringDup(arg);
 argv[nargs].has_wildcard = stdargs[i].has_wildcard;
 nargs++;
 }


Thanks
Kumar

On 5/19/2015 6:58 AM, Rob McKenna wrote:

Hi folks,

Because of platform specifics the Java launcher contains some extra
wildcard expansion processing on Windows.

As part of this processing the list of args received by
CreateApplicationArgs (java_md.c) is compared to the original list in
the java launchers main method.

Unfortunately the CreateApplicationArgs list has already been filtered
by TranslateApplicationArgs which removes VM specific flags. This
results in the launcher incorrectly neglecting to expand wildcard
arguments.

This fix filters the main method args in the same way so
CreateApplicationArgs will be comparing like with like.

http://cr.openjdk.java.net/~robm/8077822/webrev.01/

-Rob

RFR: 7130985: Four helper classes missing in Sun JDK

[Rob McKenna]

Meant to get this sorted a while back.

There was a thread on this last year ( 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-July/027779.html )


A test has been added since then:

http://cr.openjdk.java.net/~robm/7130985/webrev.corba/
http://cr.openjdk.java.net/~robm/7130985/webrev.j2se/

-Rob

Re: RFR: 7130985: Four helper classes missing in Sun JDK

[Rob McKenna]

Added some cleanup code around the BufferedReaders & the leftover test 
files:


http://cr.openjdk.java.net/~robm/7130985/webrev.02/

-Rob

On 03/06/15 16:20, Rob McKenna wrote:

Meant to get this sorted a while back.

There was a thread on this last year (
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-July/027779.html
)

A test has been added since then:

http://cr.openjdk.java.net/~robm/7130985/webrev.corba/
http://cr.openjdk.java.net/~robm/7130985/webrev.j2se/

 -Rob

Re: [8u60] approval request for JDK-8062198: Add RowSetMetaDataImpl Tests and add column range validation to isdefinitlyWritable

[Rob McKenna]

Approved.

-Rob

On 05/06/15 16:39, Maxim Soloviev wrote:

Please approve direct backport.

bug: https://bugs.openjdk.java.net/browse/JDK-8062198
jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/4162bcc663dc

Thanks,
Maxim

Re: [8u60] approval request for JDK-8059411: RowSetWarning does not correctly chain warnings

[Rob McKenna]

Approved.

-Rob

On 05/06/15 16:45, Maxim Soloviev wrote:

Please approve direct backport.

bug: https://bugs.openjdk.java.net/browse/JDK-8059411
jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/4110a7627857

Thanks,
Maxim

Re: [8u60] approval request for JDK-8066188: BaseRowSet returns the wrong default value for escape processing

[Rob McKenna]

Approved.

-Rob

On 05/06/15 16:50, Maxim Soloviev wrote:

Please approve direct backport.

bug: https://bugs.openjdk.java.net/browse/JDK-8066188
jdk9 change set: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/f619341171c0

Thanks,
Maxim

[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Rob McKenna]

This recently received CSR approval, so it seems like a good time to pick
the codereview up again:

http://cr.openjdk.java.net/~robm/8160768/webrev.08/

Referencing:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html

1) I'm copying the behaviour from
LdapCtxFactory.getInitialContext(Hashtable) where an empty String is
the default value used when the provider url is null.

I don't think HostnameChecker (by way of SNIHostname) will accept either
empty or null values when doing the comparison.

Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
appears to pass the String "null" to
StartTlsResponseImpl.setConnection(Connection, String), which attempts
to substitute null values with the Connections host so there may be a bug
here.

I'm happy to allow null returns here if necessary. Sean, can you
comment on the distinction between null & "" as far as hostname
verification is concerned?

2) In the latest iteration lookupEndpoints() returns an
Optional. Currently neither getEndpoints() nor
getDomainName() can be null. (they can be an empty list and/or an empty
String however)

3) Corrected.

4) See https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5

-Rob

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Rob McKenna]

Thanks Daniel:

http://cr.openjdk.java.net/~robm/8139965/webrev.03/

I'm planning to follow up on the test side of things with a separate
bug. I think the technique used in some of the recent SQE LDAP tests
might be applicable.

-Rob

On 05/09/18 09:53, Daniel Fuchs wrote:
> Hi Rob,
> 
> That looks better but I believe that:
> 
> 1. closed should be volatile since it's read from outside
>synchronized block
> 
> 2. it seems there might be a race where the last response
>received could be dropped, if the connection is closed
>just after it's been pulled from the queue.
> 
> So I would suggest exchanging:
> 
>  115 if (isClosed()) {
>  116 return null;
>  117 }
>  118
>  119 return result;
> 
> with:
> 
>  return result == EOF ? null : result;
> 
> best regards,
> 
> -- daniel
> 
> On 05/09/2018 02:05, Rob McKenna wrote:
> > Thanks for the reviews folks.
> > 
> > I believe the following captures your recommended changes:
> > 
> > http://cr.openjdk.java.net/~robm/8139965/webrev.02/
> > 
> > W.r.t. testing I think this area has been difficult to test
> > traditionally. I'll have a dig through the existing testbase (and I'll
> > get back to you) to see if there's anything similar but afaik most tests
> > simply mimic a bindable dummy ldap server.
> > 
> > Vyom, are you aware of any more rigorous tests / ldap test frameworks?
> > 
> >  -Rob
> > 
> > On 04/09/18 10:22, Daniel Fuchs wrote:
> > > Hi Rob,
> > > 
> > > I concur with Chris.
> > > completed needs to be volatile and close() needs to
> > > set a flag and use offer like cancel().
> > > 
> > > The condition for testing for closed then becomes
> > > that the flag is set and the queue is empty or has EOF
> > > as its head.
> > > 
> > > Is there any way this could be tested by a regression
> > > test?
> > > 
> > > best regards,
> > > 
> > > -- daniel
> > > 
> > > On 04/09/2018 10:00, Chris Hegarty wrote:
> > > > Rob,
> > > > 
> > > > > On 3 Sep 2018, at 22:48, Rob McKenna  wrote:
> > > > > 
> > > > > Hi folks,
> > > > > 
> > > > > I'd like to get a re-review of this change:
> > > > > 
> > > > > https://bugs.openjdk.java.net/browse/JDK-8139965 
> > > > > <https://bugs.openjdk.java.net/browse/JDK-8139965>
> > > > 
> > > > This issue is closed as `will not fix`. I presume you will re-open it 
> > > > before pushing.
> > > > 
> > > > > http://cr.openjdk.java.net/~robm/8139965/webrev/ 
> > > > > <http://cr.openjdk.java.net/~robm/8139965/webrev/>
> > > > 
> > > > 
> > > > 43 private boolean completed;
> > > > Won’t `completed` need to be volatile now? ( since the removal of 
> > > > synchronized from hasSearchCompleted )
> > > > 
> > > > LdapRequest.close puts EOF into the queue, but that is a potentially 
> > > > blocking operation ( while holding the lock ). If the queue is at 
> > > > capacity, then it will block forever. This model will only work if 
> > > > `getReplyBer` is always guaranteed to be running concurrently. Is it?
> > > > 
> > > > Please check the indentation of LdapRequest.java L103 ( in
> > > > the new file ). It appears, in the webrev, that the trailing `}` is
> > > > not lined up.
> > > > 
> > > > The indentation doesn’t look right here either.
> > > >624 if (nparent) {
> > > >625 LdapRequest ldr = pendingRequests;
> > > >626 while (ldr != null) {
> > > >627 ldr.close();
> > > >628 ldr = ldr.next;
> > > >629 }
> > > >630 }
> > > >631 }
> > > > 
> > > > -Chris
> > > > 
> > > 
> 

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Rob McKenna]

Thanks again Daniel,

http://cr.openjdk.java.net/~robm/8139965/webrev.04/

-Rob

On 26/10/18 09:54, Daniel Fuchs wrote:
> Hi Rob,
> 
> Now I'm concerned about this method:
> 
>   71 synchronized boolean addReplyBer(BerDecoder ber) {
>   72 // check the closed boolean value here as we don't want
> anything
>   73 // to be added to the queue after close() has been called.
>   74 if (cancelled || closed) {
>   75 return false;
>   76 }
>   77
>   78 // Add a new reply to the queue of unprocessed replies.
>   79 try {
>   80 replies.put(ber);
>   81 } catch (InterruptedException e) {
>   82 // ignore
>   83 }
>   84
>   85 // peek at the BER buffer to check if it is a SearchResultDone
> PDU
>   86 try {
>   87 ber.parseSeq(null);
>   88 ber.parseInt();
>   89 completed = (ber.peekByte() == LdapClient.LDAP_REP_RESULT);
>   90 } catch (IOException e) {
>   91 // ignore
>   92 }
>   93 ber.reset();
>   94 return pauseAfterReceipt;
>   95 }
> 
> getReplyBer() is not synchronized, so AFAICS there is a
> chance that line 93 executes after another thread as got
> hold of the `ber` object.
> 
> Is that an issue? Should the `ber` object be added to
> the reply queue only after it's been reset?
> 
> best regards,
> 
> -- daniel
> 
> On 25/10/2018 21:53, Rob McKenna wrote:
> > Thanks Daniel:
> > 
> > http://cr.openjdk.java.net/~robm/8139965/webrev.03/
> > 
> > I'm planning to follow up on the test side of things with a separate
> > bug. I think the technique used in some of the recent SQE LDAP tests
> > might be applicable.
> > 
> >  -Rob
> > 
> > On 05/09/18 09:53, Daniel Fuchs wrote:
> > > Hi Rob,
> > > 
> > > That looks better but I believe that:
> > > 
> > > 1. closed should be volatile since it's read from outside
> > > synchronized block
> > > 
> > > 2. it seems there might be a race where the last response
> > > received could be dropped, if the connection is closed
> > > just after it's been pulled from the queue.
> > > 
> > > So I would suggest exchanging:
> > > 
> > >   115 if (isClosed()) {
> > >   116 return null;
> > >   117 }
> > >   118
> > >   119 return result;
> > > 
> > > with:
> > > 
> > >   return result == EOF ? null : result;
> > > 
> > > best regards,
> > > 
> > > -- daniel
> > > 
> > > On 05/09/2018 02:05, Rob McKenna wrote:
> > > > Thanks for the reviews folks.
> > > > 
> > > > I believe the following captures your recommended changes:
> > > > 
> > > > http://cr.openjdk.java.net/~robm/8139965/webrev.02/
> > > > 
> > > > W.r.t. testing I think this area has been difficult to test
> > > > traditionally. I'll have a dig through the existing testbase (and I'll
> > > > get back to you) to see if there's anything similar but afaik most tests
> > > > simply mimic a bindable dummy ldap server.
> > > > 
> > > > Vyom, are you aware of any more rigorous tests / ldap test frameworks?
> > > > 
> > > >   -Rob
> > > > 
> > > > On 04/09/18 10:22, Daniel Fuchs wrote:
> > > > > Hi Rob,
> > > > > 
> > > > > I concur with Chris.
> > > > > completed needs to be volatile and close() needs to
> > > > > set a flag and use offer like cancel().
> > > > > 
> > > > > The condition for testing for closed then becomes
> > > > > that the flag is set and the queue is empty or has EOF
> > > > > as its head.
> > > > > 
> > > > > Is there any way this could be tested by a regression
> > > > > test?
> > > > > 
> > > > > best regards,
> > > > > 
> > > > > -- daniel
> > > > > 
> > > > > On 04/09/2018 10:00, Chris Hegarty wrote:
> > > > > > Rob,
> > > > > > 
> > > > > > > On 3 Sep 2018, at 22:48, Rob McKenna  
> > > > > > > wrote:
> > > > > > > 
> > > > > > > Hi folks,
> > > > > > > 

Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Rob McKenna]

Yes, thanks a lot Alan.

Vyom and Martin both had some very helpful feedback so I'm hoping to
hear from both!

-Rob

On 26/10/18 15:34, Alan Bateman wrote:
> On 25/10/2018 17:34, Rob McKenna wrote:
> > This recently received CSR approval, so it seems like a good time to pick
> > the codereview up again:
> > 
> > http://cr.openjdk.java.net/~robm/8160768/webrev.08/
> > 
> I went through a number of iterations with Rob on the API/javadoc so I think
> the API parts in webrev.08 are good. I have not had time to go through the
> implementation and test changes so hopefully that someone else has cycles to
> help on that.
> 
> -Alan
> 

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Rob McKenna]

On 26/10/18 16:14, Daniel Fuchs wrote:
> Hi Rob,
> 
> Looks better to me know. Though I admit that:
> 
> 53 this.replies = new LinkedBlockingQueue<>(8 *
> replyQueueCapacity / 10);
> 
> is still a bit mystifying... Why not use the full
> replyQueueCapacity provided? That doesn't look
> strictly equivalent to the highWatermark logic that
> you have removed.

Pavel may be able to shed more light on this part. (as it was his
suggested fix originally I believe)

It looks to me like the intention is to mimic the pre-fix behaviour,
which only allowed the queue to fill to 80% of its capacity before
pausing. I agree though, it doesn't really make sense to keep that. I'll
remove it.

> 
> On 25/10/2018 21:53, Rob McKenna wrote:
> > I'm planning to follow up on the test side of things with a separate
> > bug. I think the technique used in some of the recent SQE LDAP tests
> > might be applicable.
> 
> It will be good to have a test and try to shake the implementation
> a bit with some repeating jobs in our test system to get some
> confidence that we've not harmed anything else.

You've convinced me to wait until we can get one together. I'll hold off
on the push until that point. jdk-tier1-4 pass, but that may not be
saying much unfortunately.

-Rob

> 
> I admit that my only acquaintance to the JNDI/LDAP code of the JDK
> has been through reviews, so I'd probably only spot the obvious.
> 
> best regards,
> 
> -- daniel
> 
> 
> On 26/10/2018 15:55, Rob McKenna wrote:
> > Thanks again Daniel,
> > 
> > http://cr.openjdk.java.net/~robm/8139965/webrev.04/
> > 
> >  -Rob
> > 
> 

Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Rob McKenna]

Thanks folks,

Vyom, I've updated service to be volatile.

On 30/10/18 14:25, Daniel Fuchs wrote:
> Hi Rob,
> 
> LdapCtxFactory.java
> 
>  187 for (String u : r.getEndpoints()) {
>  188 try {
>  189 ctx = getLdapCtxFromUrl(
>  190 r.getDomainName(), new LdapURL(u), env);
>  191 } catch (NamingException e) {
>  192 // try the next element
>  193 lastException = e;
>  194 }
>  195 }
> 
> is a break statement missing after line 190?
> 
> If not then can you add a comment explaining why only the last
> context is retained (and returned?)
> 
> Alternatively, if a break is indeed missing, these three lines
> could be moved into the for loop above:
> 
>  206 // Record the URL that created the context
>  207 ctx.setProviderUrl(url);
>  208 return ctx;
> 
> and maybe lines 206-207 could be moved into the
> getLdapCtxFromUrl() method?
> 

Yes, you're right, we should return the first successful result.

> LdapDnsProviderService.java:
> 
> Why is this class non final? If it can be made final then
> the protected methods should probably become package.
> 

Good point, fixed.

> LdapDnsProvider.java:
> 
> It is strange to see new APIs with Hashtable in the method
> signature. I understand that our implementation will need
> an Hashtable at some point to call javax.naming.spi.NamingProvider,
> but how likely is it that a clean room implementation of
> a LdapDnsProvider will need to do that?
> 
> Maybe we could have Map in the signature instead - and
> leave the burden to our implementation - somewhere in ServiceLocator,
> to adapt back to Hashtable where it needs to?

So I've altered the signature of the method to take a Map as
proposed. I've added a getLdapService(String domainName, Map environment)
method to ServiceLocator which delegates to the existing method after
conversion. Hopefully this addresses your concerns.

I'll update the CSR accordingly once this review is complete.

-Rob

> 
> 
> best regards,
> 
> -- daniel
> 
> 
> On 25/10/2018 17:34, Rob McKenna wrote:
> > This recently received CSR approval, so it seems like a good time to pick
> > the codereview up again:
> > 
> > http://cr.openjdk.java.net/~robm/8160768/webrev.08/
> > 
> > Referencing:
> > 
> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html
> > 
> > 1) I'm copying the behaviour from
> > LdapCtxFactory.getInitialContext(Hashtable) where an empty String is
> > the default value used when the provider url is null.
> > 
> > I don't think HostnameChecker (by way of SNIHostname) will accept either
> > empty or null values when doing the comparison.
> > 
> > Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
> > appears to pass the String "null" to
> > StartTlsResponseImpl.setConnection(Connection, String), which attempts
> > to substitute null values with the Connections host so there may be a bug
> > here.
> > 
> > I'm happy to allow null returns here if necessary. Sean, can you
> > comment on the distinction between null & "" as far as hostname
> > verification is concerned?
> > 
> > 2) In the latest iteration lookupEndpoints() returns an
> > Optional. Currently neither getEndpoints() nor
> > getDomainName() can be null. (they can be an empty list and/or an empty
> > String however)
> > 
> > 3) Corrected.
> > 
> > 4) See https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5
> > 
> >  -Rob
> > 
> 

Re: [RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[Rob McKenna]

You've cracked my elabourate webrev naming scheme. Thanks Daniel,

On 06/11/18 16:39, Daniel Fuchs wrote:
> Hi Rob,
> 
> Assuming the new webrev is:
> 
> http://cr.openjdk.java.net/~robm/8160768/webrev.09
> 
> then it looks much better to me.
> 
> I haven't re-reviewed everything in details -  just looked
> at the updates (LdapDnsProviderService,
> LdapDnsProvider signature change, ServiceLocator)
> 

Yep, apart from the volatile service nothing else has changed.

> BTW: if I'm not mistaken returning an empty Optional is
> equivalent, in the end, to returning an Optional containing
> a LdapDnsProviderResult which returns an empty list of endpoints?
> I guess that's OK, I don't see any value in preventing an
> LdapDnsProviderResult to have an empty endpoint list anyway.
> 

Basically, yes.

Thanks,

    -Rob

> best regards,
> 
> -- daniel
> 
> 
> On 06/11/2018 15:52, Rob McKenna wrote:
> > Thanks folks,
> > 
> > Vyom, I've updated service to be volatile.
> > 
> > On 30/10/18 14:25, Daniel Fuchs wrote:
> > > Hi Rob,
> > > 
> > > LdapCtxFactory.java
> > > 
> > >   187 for (String u : r.getEndpoints()) {
> > >   188 try {
> > >   189 ctx = getLdapCtxFromUrl(
> > >   190 r.getDomainName(), new LdapURL(u), env);
> > >   191 } catch (NamingException e) {
> > >   192 // try the next element
> > >   193 lastException = e;
> > >   194 }
> > >   195 }
> > > 
> > > is a break statement missing after line 190?
> > > 
> > > If not then can you add a comment explaining why only the last
> > > context is retained (and returned?)
> > > 
> > > Alternatively, if a break is indeed missing, these three lines
> > > could be moved into the for loop above:
> > > 
> > >   206 // Record the URL that created the context
> > >   207 ctx.setProviderUrl(url);
> > >   208 return ctx;
> > > 
> > > and maybe lines 206-207 could be moved into the
> > > getLdapCtxFromUrl() method?
> > > 
> > 
> > Yes, you're right, we should return the first successful result.
> > 
> > > LdapDnsProviderService.java:
> > > 
> > > Why is this class non final? If it can be made final then
> > > the protected methods should probably become package.
> > > 
> > 
> > Good point, fixed.
> > 
> > > LdapDnsProvider.java:
> > > 
> > > It is strange to see new APIs with Hashtable in the method
> > > signature. I understand that our implementation will need
> > > an Hashtable at some point to call javax.naming.spi.NamingProvider,
> > > but how likely is it that a clean room implementation of
> > > a LdapDnsProvider will need to do that?
> > > 
> > > Maybe we could have Map in the signature instead - and
> > > leave the burden to our implementation - somewhere in ServiceLocator,
> > > to adapt back to Hashtable where it needs to?
> > 
> > So I've altered the signature of the method to take a Map as
> > proposed. I've added a getLdapService(String domainName, Map 
> > environment)
> > method to ServiceLocator which delegates to the existing method after
> > conversion. Hopefully this addresses your concerns.
> > 
> > I'll update the CSR accordingly once this review is complete.
> > 
> >  -Rob
> > 
> > > 
> > > 
> > > best regards,
> > > 
> > > -- daniel
> > > 
> > > 
> > > On 25/10/2018 17:34, Rob McKenna wrote:
> > > > This recently received CSR approval, so it seems like a good time to 
> > > > pick
> > > > the codereview up again:
> > > > 
> > > > http://cr.openjdk.java.net/~robm/8160768/webrev.08/
> > > > 
> > > > Referencing:
> > > > 
> > > > http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html
> > > > 
> > > > 1) I'm copying the behaviour from
> > > > LdapCtxFactory.getInitialContext(Hashtable) where an empty String 
> > > > is
> > > > the default value used when the provider url is null.
> > > > 
> > > > I don't think HostnameChecker (by way of SNIHostname) will accept either
> > > > empty or null values when doing the comparison.
> > > > 
> > > > Somewhat oddly however, LdapCtx.extendedOperation(ExtendedRequest)
> > > > appears to pass the String "null" to
> > > > StartTlsResponseImpl.setConnection(Connection, String), which attempts
> > > > to substitute null values with the Connections host so there may be a 
> > > > bug
> > > > here.
> > > > 
> > > > I'm happy to allow null returns here if necessary. Sean, can you
> > > > comment on the distinction between null & "" as far as hostname
> > > > verification is concerned?
> > > > 
> > > > 2) In the latest iteration lookupEndpoints() returns an
> > > > Optional. Currently neither getEndpoints() nor
> > > > getDomainName() can be null. (they can be an empty list and/or an empty
> > > > String however)
> > > > 
> > > > 3) Corrected.
> > > > 
> > > > 4) See 
> > > > https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5
> > > > 
> > > >   -Rob
> > > > 
> > > 
> 

[RFR] 8214440: ldap over a TLS connection negotiate failed with "javax.net.ssl.SSLPeerUnverifiedException: hostname of the server '' does not match the hostname in the server's certificate"

[Rob McKenna]

Hi folks,

I'd like to fix this test failure caused by 8160768.

The problem is that the LdapDnsProviderResult sets the hostname to the
empty String and gets passed to StartTlsResponseImpl.verify.
Unfortunately StartTlsResponseImpl.verify only expects null values.
Since null and the empty String are functionally equivalent I've added a
check to StartTlsResponseImpl.verify to take the empty String into
account.

http://cr.openjdk.java.net/~robm/8214440/webrev.01/

This was caught by an existing test which I managed to miss in my
testing incantation.

-Rob

Re: [RFR] 8214440: ldap over a TLS connection negotiate failed with "javax.net.ssl.SSLPeerUnverifiedException: hostname of the server '' does not match the hostname in the server's certificate"

[Rob McKenna]

The parameter can be null, but if you look at the spec for getDomainName
it details the behaviour when the result is created with a null value.

I would rather avoid changing the spec at this point so I plan to stick
with this approach. (I'll switch the "".equals for isEmpty - thanks for
that)

-Rob

On 09/01/19 12:33, vyom tewari wrote:
> Hi Rob,
> 
> Thanks for fixing this issue, please use hostname.isEmpty(), instead of
> "".equal(hostname). I have a question to you, why we are converting null to
> empty string("") in LdapDnsProvider ?.
> 
> If you see the java doc it tells that domainname can be null
> 
> /**
>  * Construct an LdapDnsProviderResult consisting of a resolved domain
> name
>  * and the ldap server endpoints that serve the domain.
>  *
>  * @param domainName    the resolved domain name; can be null.
> 
> My personal suggestion is not to replace null to empty string("") in
> "LdapDnsProviderResult" either throw some exception or just pass whatever
> you got in LdapDnsProviderResult constructor.
> 
> Thanks,
> 
> Vyom
> 
> On 08/01/19 10:33 PM, Rob McKenna wrote:
> > Hi folks,
> > 
> > I'd like to fix this test failure caused by 8160768.
> > 
> > The problem is that the LdapDnsProviderResult sets the hostname to the
> > empty String and gets passed to StartTlsResponseImpl.verify.
> > Unfortunately StartTlsResponseImpl.verify only expects null values.
> > Since null and the empty String are functionally equivalent I've added a
> > check to StartTlsResponseImpl.verify to take the empty String into
> > account.
> > 
> > http://cr.openjdk.java.net/~robm/8214440/webrev.01/
> > 
> > This was caught by an existing test which I managed to miss in my
> > testing incantation.
> > 
> >  -Rob
> > 

RFR - 8143640: Showing incorrect result while passing specific argument in the Java launcher tool

[Rob McKenna]

Hi folks,

Looking for a review for this simple change:

http://cr.openjdk.java.net/~robm/8143640/webrev.01/

Basically the windows command parser was inserting extra slashes under certain 
circumstances.

cmdtoargs.c standalone test passes, as do the regression tests.

-Rob

RFR - 8141148: LDAP "follow" throws ClassCastException with Java 8

[Rob McKenna]

Hi folks,

Looking for a review for this change:

https://bugs.openjdk.java.net/browse/JDK-8141148
http://cr.openjdk.java.net/~robm/8141148/webrev.01/

A fairly straightforward fix for a class cast problem. From the bug:

As a result of https://bugs.openjdk.java.net/browse/JDK-7072353:

http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/18329abcdb7c

The test attached to the bug fails with the following trace:

Exception in thread "main" java.lang.ClassCastException: 
com.sun.jndi.ldap.LdapSearchEnumeration cannot be cast to 
com.sun.jndi.ldap.LdapNamingEnumeration
   at 
com.sun.jndi.ldap.LdapNamingEnumeration.getReferredResults(LdapNamingEnumeration.java:78)
   at 
com.sun.jndi.ldap.LdapNamingEnumeration.getReferredResults(LdapNamingEnumeration.java:36)
   at 
com.sun.jndi.ldap.AbstractLdapNamingEnumeration.hasMoreReferrals(AbstractLdapNamingEnumeration.java:330)
   at 
com.sun.jndi.ldap.AbstractLdapNamingEnumeration.hasMoreImpl(AbstractLdapNamingEnumeration.java:227)
   at 
com.sun.jndi.ldap.AbstractLdapNamingEnumeration.hasMore(AbstractLdapNamingEnumeration.java:189)
   at 
com.sun.jndi.ldap.AbstractLdapNamingEnumeration.hasMoreElements(AbstractLdapNamingEnumeration.java:117)
   at TestLdap.query(TestLdap.java:43)
   at TestLdap.main(TestLdap.java:19)

Post refactor we have a NamingEnumeration interface which is implemented by an 
abstract class AbstractLdapNamingEnumeration.

3 implementations, LdapBindingEnumeration, LdapNamingEnumeration & 
LdapSearchEnumeration extend AbstractLdapNamingEnumeration.

Each implementation has its own getReferredResults method which returns an 
object of the individual implementations type.

Unfortunately this falls down (as can be seen in the stack) because an 
LdapNamingEnumeration.getReferredResults can call refCtx.search, which returns 
an LdapSearchEnumeration. This leads to a situation where LdapNamingEnumeration 
needs to cast an LdapSearchEnumeration which results in the exception above.

-Rob

Re: JDK 9 RFR of JDK-8166054: Problem list JarURLConnectionUseCaches.java until JDK-8165988 is fixed

[Rob McKenna]

Sorry Joe,

This passed just fine in jprt. I'll change it to othervm now. (I only noticed 
this problem on 8 last night)

-Rob

On 14/09/16 06:09, Daniel Fuchs wrote:
> Hi Joe,
> 
> Looks good to me!
> 
> -- daniel
> 
> On 14/09/16 18:07, joe darcy wrote:
> >Hello,
> >
> >Until JDK-8165988 is fixed, the test
> >
> >sun/net/www/protocol/jar/JarURLConnectionUseCaches.java
> >
> >should be problem listed on windows. Please review the patch below.
> >
> >Thanks,
> >
> >-Joe
> >
> >--- a/test/ProblemList.txtWed Sep 14 07:37:15 2016 -0700
> >+++ b/test/ProblemList.txtWed Sep 14 10:03:29 2016 -0700
> >@@ -159,7 +159,6 @@
> >
> > # jdk_net
> >
> >-
> > java/net/MulticastSocket/NoLoopbackPackets.java 7122846 macosx-all
> > java/net/MulticastSocket/SetLoopbackMode.java 7122846 macosx-all
> >
> >@@ -173,6 +172,8 @@
> > java/net/httpclient/http2/ErrorTest.java 8158127 solaris-all,windows-all
> > java/net/httpclient/http2/TLSConnection.java 8157482 macosx-all
> >
> >+sun/net/www/protocol/jar/JarURLConnectionUseCaches.java 8165988
> >windows-all
> >+
> > 
> >
> >
> > # jdk_nio
> >
> 

[8u-dev] RFR - 8156824: com.sun.jndi.ldap.pool.PoolCleaner should clear its context class loader

[Rob McKenna]

Hi folks,

I'd like to backport this change to 8u-dev.

The changes are straightforward enough but 8u needs some changes that
were made to InnocuousThread. (strictly it doesn't need all of the
changes I've made but I've made an effort to make the code look as close
to 9 as possible)

bug: https://bugs.openjdk.java.net/browse/JDK-8156824
9u changeset: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/22e704dfa05c
8u webrev: http://cr.openjdk.java.net/~robm/8156824/webrev.01/

-Rob

RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Rob McKenna]

Hi folks,

I'd like to get a re-review of this change:

https://bugs.openjdk.java.net/browse/JDK-8139965
http://cr.openjdk.java.net/~robm/8139965/webrev/

In case the original has gone stale:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042767.html

-Rob

Re: RFR: 8139965 - Hang seen when using com.sun.jndi.ldap.search.replyQueueSize

[Rob McKenna]

Thanks for the reviews folks.

I believe the following captures your recommended changes:

http://cr.openjdk.java.net/~robm/8139965/webrev.02/

W.r.t. testing I think this area has been difficult to test
traditionally. I'll have a dig through the existing testbase (and I'll
get back to you) to see if there's anything similar but afaik most tests
simply mimic a bindable dummy ldap server.

Vyom, are you aware of any more rigorous tests / ldap test frameworks?

-Rob

On 04/09/18 10:22, Daniel Fuchs wrote:
> Hi Rob,
> 
> I concur with Chris.
> completed needs to be volatile and close() needs to
> set a flag and use offer like cancel().
> 
> The condition for testing for closed then becomes
> that the flag is set and the queue is empty or has EOF
> as its head.
> 
> Is there any way this could be tested by a regression
> test?
> 
> best regards,
> 
> -- daniel
> 
> On 04/09/2018 10:00, Chris Hegarty wrote:
> > Rob,
> > 
> > > On 3 Sep 2018, at 22:48, Rob McKenna  wrote:
> > > 
> > > Hi folks,
> > > 
> > > I'd like to get a re-review of this change:
> > > 
> > > https://bugs.openjdk.java.net/browse/JDK-8139965 
> > > <https://bugs.openjdk.java.net/browse/JDK-8139965>
> > 
> > This issue is closed as `will not fix`. I presume you will re-open it 
> > before pushing.
> > 
> > > http://cr.openjdk.java.net/~robm/8139965/webrev/ 
> > > <http://cr.openjdk.java.net/~robm/8139965/webrev/>
> > 
> > 
> > 43 private boolean completed;
> > Won’t `completed` need to be volatile now? ( since the removal of 
> > synchronized from hasSearchCompleted )
> > 
> > LdapRequest.close puts EOF into the queue, but that is a potentially 
> > blocking operation ( while holding the lock ). If the queue is at capacity, 
> > then it will block forever. This model will only work if `getReplyBer` is 
> > always guaranteed to be running concurrently. Is it?
> > 
> > Please check the indentation of LdapRequest.java L103 ( in
> > the new file ). It appears, in the webrev, that the trailing `}` is
> > not lined up.
> > 
> > The indentation doesn’t look right here either.
> >   624 if (nparent) {
> >   625 LdapRequest ldr = pendingRequests;
> >   626 while (ldr != null) {
> >   627 ldr.close();
> >   628 ldr = ldr.next;
> >   629 }
> >   630 }
> >   631 }
> > 
> > -Chris
> > 
>