date:20161028


Hi Roger,

Okay we are back to where we were a couple of emails ago. :) Removing 
everything after the p.destroy() seems fine to me.


Thanks,
David

On 29/10/2016 7:16 AM, Roger Riggs wrote:

Hi,


On 10/28/2016 4:59 PM, David Holmes wrote:


But this is the second waitFor call after the process is destroyed.
Sorry I don't really see the point.

The tests were added to determine if waitFor(timeout) was
handling the
timeout parameter correctly.
The 2nd test here was to check the datapath when the process already
been destroyed.
The 'extra' waitFor() ensures the process is gone.
But you are correct, this exercises a path through the waitFor code
that
does not even look
at the timeout value so I'll remove the entire 2nd case.

Webrev updated in place.
  http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


Okay, but now you don't need the p.waitFor() after the destroy() at
all.

Hmmm ... I guess either we want the original code with a longer
timeout to accommodate slow/loaded platforms (to test that waitFor(n)
after a destroy() is timely); or everything after destroy() can be
deleted.

Here's another approach to test whether waitFor(timeout) is
behaving as
expected.
The test should be checking that waitFor does not wait longer than
requested.

So shorten the timeout to a very short time (10 milliseconds) and
check that
waitFor returns in no more than that time(plus epsilon).  It is
similar
to the first test
in that section but the process is exiting.

   http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


But the first test is checking that we wait at-least as long as the
timeout ie no early return when we know the process is still alive.

Sorry but this isn't making sense to me. The original problem is that
waitFor(8) has timed out and we proceeded to call exitValue() while
the process is still alive. If the process had in fact terminated then
we would have hit the (end-start) check and failed due to a timeout.
So what exactly are we trying to fix here? Do we simply prefer to fail
due to timeout rather than have exitValue() throw
IllegalStateException?

calling exitValue was just additional debugging info; but the test would
have failed the timeout condition later.
So the test is still broken, because it is not checking the correct
timeout behavior of waitFor(timeout) and instead
is checking how long it takes the OS to destroy.  That's not
interesting.

If so then check for waitFor(8) timing out, but leave the timeout
value at 8 seconds - just avoid the call to exitValue() if a time-out
occurred. Otherwise if we want to avoid timing out we either bump the
timeout to some value > 8 to accommodate the load problem, or we don't
even bother doing waitFor after the destroy().

Lengthening the time isn't interesting either because it is only testing
that it returns
when the process has exited, not whether the timeout is hit at the right
time.


There is no real way to test timeouts. You can check you don't get
early returns but you can't check that you return in a timely fashion.
At least without knowing the execution environment.

The spec for waitFor is " until the
 * process represented by this Process object has
 * terminated, or the specified waiting time elapses."


Does waitFor implement it's own timed-waiting mechanism, or does it
just delegate to an existing API (like wait() or park()) ? If the
latter then that is where timeout testing would exist.

Thread.sleep() on Linux with interrupt() from a monitoring thread.



The earlier part of the test already verifies both that the timeout
mechanism functions, and that it doesn't return early.


So I guess, its time to remove the whole test case on a destroyed
process.


Given destroy() is not guaranteed to work either ... testing waitFor
in that context will always be somewhat probabilistic I think - and
timeout testing just makes that more so.

ok, removed the test case.

http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger



Cheers,
David


R

Re: RFR 9: 8168517 : java/lang/ProcessBuilder/Basic.java failed


Hi,


On 10/28/2016 4:59 PM, David Holmes wrote:


But this is the second waitFor call after the process is destroyed.
Sorry I don't really see the point.
The tests were added to determine if waitFor(timeout) was 
handling the

timeout parameter correctly.
The 2nd test here was to check the datapath when the process already
been destroyed.
The 'extra' waitFor() ensures the process is gone.
But you are correct, this exercises a path through the waitFor code
that
does not even look
at the timeout value so I'll remove the entire 2nd case.

Webrev updated in place.
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


Okay, but now you don't need the p.waitFor() after the destroy() at
all.

Hmmm ... I guess either we want the original code with a longer
timeout to accommodate slow/loaded platforms (to test that waitFor(n)
after a destroy() is timely); or everything after destroy() can be
deleted.
Here's another approach to test whether waitFor(timeout) is 
behaving as

expected.
The test should be checking that waitFor does not wait longer than
requested.

So shorten the timeout to a very short time (10 milliseconds) and
check that
waitFor returns in no more than that time(plus epsilon).  It is 
similar

to the first test
in that section but the process is exiting.

http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


But the first test is checking that we wait at-least as long as the
timeout ie no early return when we know the process is still alive.

Sorry but this isn't making sense to me. The original problem is that
waitFor(8) has timed out and we proceeded to call exitValue() while
the process is still alive. If the process had in fact terminated then
we would have hit the (end-start) check and failed due to a timeout.
So what exactly are we trying to fix here? Do we simply prefer to fail
due to timeout rather than have exitValue() throw 
IllegalStateException?

calling exitValue was just additional debugging info; but the test would
have failed the timeout condition later.
So the test is still broken, because it is not checking the correct
timeout behavior of waitFor(timeout) and instead
is checking how long it takes the OS to destroy.  That's not 
interesting.

If so then check for waitFor(8) timing out, but leave the timeout
value at 8 seconds - just avoid the call to exitValue() if a time-out
occurred. Otherwise if we want to avoid timing out we either bump the
timeout to some value > 8 to accommodate the load problem, or we don't
even bother doing waitFor after the destroy().

Lengthening the time isn't interesting either because it is only testing
that it returns
when the process has exited, not whether the timeout is hit at the right
time.


There is no real way to test timeouts. You can check you don't get 
early returns but you can't check that you return in a timely fashion. 
At least without knowing the execution environment.

The spec for waitFor is " until the
 * process represented by this Process object has
 * terminated, or the specified waiting time elapses."


Does waitFor implement it's own timed-waiting mechanism, or does it 
just delegate to an existing API (like wait() or park()) ? If the 
latter then that is where timeout testing would exist.

Thread.sleep() on Linux with interrupt() from a monitoring thread.



The earlier part of the test already verifies both that the timeout 
mechanism functions, and that it doesn't return early.


So I guess, its time to remove the whole test case on a destroyed 
process.


Given destroy() is not guaranteed to work either ... testing waitFor 
in that context will always be somewhat probabilistic I think - and 
timeout testing just makes that more so.

ok, removed the test case.

http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger



Cheers,
David


R

Re: RFR 9: 8168517 : java/lang/ProcessBuilder/Basic.java failed


On 29/10/2016 5:39 AM, Roger Riggs wrote:

Hi,

On 10/28/2016 3:14 PM, David Holmes wrote:

On 29/10/2016 3:48 AM, Roger Riggs wrote:

Hi David,

On 10/27/2016 9:00 PM, David Holmes wrote:

But this is the second waitFor call after the process is destroyed.
Sorry I don't really see the point.

The tests were added to determine if waitFor(timeout) was handling the
timeout parameter correctly.
The 2nd test here was to check the datapath when the process already
been destroyed.
The 'extra' waitFor() ensures the process is gone.
But you are correct, this exercises a path through the waitFor code
that
does not even look
at the timeout value so I'll remove the entire 2nd case.

Webrev updated in place.
  http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


Okay, but now you don't need the p.waitFor() after the destroy() at
all.

Hmmm ... I guess either we want the original code with a longer
timeout to accommodate slow/loaded platforms (to test that waitFor(n)
after a destroy() is timely); or everything after destroy() can be
deleted.

Here's another approach to test whether waitFor(timeout) is behaving as
expected.
The test should be checking that waitFor does not wait longer than
requested.

So shorten the timeout to a very short time (10 milliseconds) and
check that
waitFor returns in no more than that time(plus epsilon).  It is similar
to the first test
in that section but the process is exiting.

   http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


But the first test is checking that we wait at-least as long as the
timeout ie no early return when we know the process is still alive.

Sorry but this isn't making sense to me. The original problem is that
waitFor(8) has timed out and we proceeded to call exitValue() while
the process is still alive. If the process had in fact terminated then
we would have hit the (end-start) check and failed due to a timeout.
So what exactly are we trying to fix here? Do we simply prefer to fail
due to timeout rather than have exitValue() throw IllegalStateException?

calling exitValue was just additional debugging info; but the test would
have failed the timeout condition later.
So the test is still broken, because it is not checking the correct
timeout behavior of waitFor(timeout) and instead
is checking how long it takes the OS to destroy.  That's not interesting.

If so then check for waitFor(8) timing out, but leave the timeout
value at 8 seconds - just avoid the call to exitValue() if a time-out
occurred. Otherwise if we want to avoid timing out we either bump the
timeout to some value > 8 to accommodate the load problem, or we don't
even bother doing waitFor after the destroy().

Lengthening the time isn't interesting either because it is only testing
that it returns
when the process has exited, not whether the timeout is hit at the right
time.


There is no real way to test timeouts. You can check you don't get early 
returns but you can't check that you return in a timely fashion. At 
least without knowing the execution environment.


Does waitFor implement it's own timed-waiting mechanism, or does it just 
delegate to an existing API (like wait() or park()) ? If the latter then 
that is where timeout testing would exist.


The earlier part of the test already verifies both that the timeout 
mechanism functions, and that it doesn't return early.



So I guess, its time to remove the whole test case on a destroyed process.


Given destroy() is not guaranteed to work either ... testing waitFor in 
that context will always be somewhat probabilistic I think - and timeout 
testing just makes that more so.


Cheers,
David


Roger



I'm really unclear what this is trying to test, and what a failure
would indicate.

Thanks,
David


Thanks, Roger




Cheers,
David



Thanks, Roger

David



Roger



David


I contemplated increasing the timeout but given the issue is
system
loading
I didn't have a good idea what to raise it to.  30sec, 1 min, 2
min.
and the harness timeout is 2min.

Roger



David


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger

Re: JEP 293: Guidelines for JDK Command-Line Tool Options - @-files

2016-10-28 Thread Alan Bateman




On 28/10/2016 20:15, Robert Scholte wrote:

:

Now I'm getting new messages, often one of these:
Error: module-info.class not found for hibernate.jpa module
or
Error: module-info.class not found for spring.context module
or
Error: module-info.class not found for spring.orm module

So let's forget about the space, might be a corrupted file in my 
repository, can't explain the exception though.


Now I'm blocked by these errors. Yes, it is true that these modules 
don't have a module-info.class, but that shouldn't be a problem, 
right? Compiling the project with my module-info files went fine...
If there isn't a module-info.class in the root directory of these JAR 
files then they are considered to be automatic modules. Automatic 
modules can't be linked into a runtime image. I realize this isn't the 
main point of your thread but just mentioning it that you will always 
get an error from link when there is a dependency on an automatic module.


-Alan

Re: RFR 9: 8168517 : java/lang/ProcessBuilder/Basic.java failed


Hi,

On 10/28/2016 3:14 PM, David Holmes wrote:

On 29/10/2016 3:48 AM, Roger Riggs wrote:

Hi David,

On 10/27/2016 9:00 PM, David Holmes wrote:

But this is the second waitFor call after the process is destroyed.
Sorry I don't really see the point.

The tests were added to determine if waitFor(timeout) was handling the
timeout parameter correctly.
The 2nd test here was to check the datapath when the process already
been destroyed.
The 'extra' waitFor() ensures the process is gone.
But you are correct, this exercises a path through the waitFor code 
that

does not even look
at the timeout value so I'll remove the entire 2nd case.

Webrev updated in place.
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


Okay, but now you don't need the p.waitFor() after the destroy() at 
all.


Hmmm ... I guess either we want the original code with a longer
timeout to accommodate slow/loaded platforms (to test that waitFor(n)
after a destroy() is timely); or everything after destroy() can be
deleted.

Here's another approach to test whether waitFor(timeout) is behaving as
expected.
The test should be checking that waitFor does not wait longer than
requested.

So shorten the timeout to a very short time (10 milliseconds) and 
check that

waitFor returns in no more than that time(plus epsilon).  It is similar
to the first test
in that section but the process is exiting.

http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


But the first test is checking that we wait at-least as long as the 
timeout ie no early return when we know the process is still alive.


Sorry but this isn't making sense to me. The original problem is that 
waitFor(8) has timed out and we proceeded to call exitValue() while 
the process is still alive. If the process had in fact terminated then 
we would have hit the (end-start) check and failed due to a timeout. 
So what exactly are we trying to fix here? Do we simply prefer to fail 
due to timeout rather than have exitValue() throw IllegalStateException?
calling exitValue was just additional debugging info; but the test would 
have failed the timeout condition later.
So the test is still broken, because it is not checking the correct 
timeout behavior of waitFor(timeout) and instead

is checking how long it takes the OS to destroy.  That's not interesting.
If so then check for waitFor(8) timing out, but leave the timeout 
value at 8 seconds - just avoid the call to exitValue() if a time-out 
occurred. Otherwise if we want to avoid timing out we either bump the 
timeout to some value > 8 to accommodate the load problem, or we don't 
even bother doing waitFor after the destroy().
Lengthening the time isn't interesting either because it is only testing 
that it returns
when the process has exited, not whether the timeout is hit at the right 
time.


So I guess, its time to remove the whole test case on a destroyed process.

Roger



I'm really unclear what this is trying to test, and what a failure 
would indicate.


Thanks,
David


Thanks, Roger




Cheers,
David



Thanks, Roger

David



Roger



David

I contemplated increasing the timeout but given the issue is 
system

loading
I didn't have a good idea what to raise it to. 30sec, 1 min, 2 
min.

and the harness timeout is 2min.

Roger



David


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger

Re: RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError


Hi Daniel,

I've read the bug report on this and this issue "smells" to me. 
LinkageError should not be a special case IMHO. The existing code was 
trying to go the "ignore everything but 'serious errors' " route - but 
without really considering what constitutes a "serious error". I think 
the fact a LinkageError can turn up here reflects a bug in the code that 
throws it - or possibly in something in between.


There should be a very clear exception handling policy for this code, 
and not special cases IMHO. Having that policy depend on whether 
shutdown is in progress is okay to me as we know that things can behave 
unexpectedly in that case. So I would advocate for a more general


+ } catch (Throwable e) {
+ // Ignore all exceptions while shutting down
+ if (globalHandlersState != STATE_SHUTDOWN) {
+ throw e;
+ }

Cheers,
David

On 28/10/2016 9:51 PM, Daniel Fuchs wrote:

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: JEP 293: Guidelines for JDK Command-Line Tool Options - @-files

2016-10-28 Thread Robert Scholte

On Fri, 28 Oct 2016 07:05:06 +0200, Henry Jen wrote:

OS-specific encoding, but has to be ASCII friendly, modern system with
UTF-8 as system encoding should work just fine.

Hmm, this will probably work for 99%, but I'm sure that we will have Maven
users which use special characters on their system and they will hit this
problem and put it on our mailinglist. I would really like a complete
answer which, for instance, makes it possible to refer to any file on the
system. Even if the answer is: use ISO 8859-1 and unicode escaping I
would be happy.

btw, it seems like my modern Windows 10 still uses Cp1252

Space in quote should work just fine, for example, “c:\\Program Files”
should be correct. Can you post messages from JLink? Also if you can
verify java is working OK with space, that would be helpful to tell if
there is something different in JLink.

I got this message:
Error: Module velocity not found, required by simple.command

Just to be sure it is not my system I cleared my local repository.

Now I'm getting new messages, often one of these:
Error: module-info.class not found for hibernate.jpa module
or
Error: module-info.class not found for spring.context module
or
Error: module-info.class not found for spring.orm module

So let's forget about the space, might be a corrupted file in my
repository, can't explain the exception though.

Now I'm blocked by these errors. Yes, it is true that these modules don't
have a module-info.class, but that shouldn't be a problem, right?
Compiling the project with my module-info files went fine...

thanks,
Robert

Cheers,
Henry

On October 27, 2016 at 2:27:44 PM, Robert Scholte (rfscho...@apache.org)
wrote:

Hi,

I'm facing some troubles with the content of the @-files and the
documentation[1] isn't helping me yet.

First of all it doesn't mention the encoding, I assume it is the OS
specific encoding.

I'm facing issues with a long --module-path on Windows.
I noticed I can use the "normal" filename (not a URI or URL), but I need
to escape the \ with an \, resulting in
"E:\\java-workspace\\sandbox\\mvnexbook-examples-1.0\\ch-multi-spring\\simple-parent\\simple-command\\target\\classes;E:\\java-workspace\\sandbox\\mvnexbook-examples-1.0\\ch-multi-spring\\simple-parent\\simple-weather\\target\\simple-weather-0.8-SNAPSHOT.jar;"
(and much more entries)

You can use a space or a newline as separator between arguments, but if
an

argument contains spaces it must be surrounded with double quotes.
However, the path to my local (Maven) repository contains a space and
JLink is complaining it cannot find these dependencies. It seems like I
need to escape spaces too, though I haven't figured out how to do that.
Changing my local repo to a space-less path works. There should be a way
to handle spaces, but how?

I'm not sure if there are other specific things to keep in mind when
writing @-files, but it would be nice if that would be explained it the
jep too.

thanks,
Robert

[1] http://openjdk.java.net/jeps/293

Re: RFR 9: 8168517 : java/lang/ProcessBuilder/Basic.java failed


On 29/10/2016 3:48 AM, Roger Riggs wrote:

Hi David,

On 10/27/2016 9:00 PM, David Holmes wrote:

But this is the second waitFor call after the process is destroyed.
Sorry I don't really see the point.

The tests were added to determine if waitFor(timeout) was handling the
timeout parameter correctly.
The 2nd test here was to check the datapath when the process already
been destroyed.
The 'extra' waitFor() ensures the process is gone.
But you are correct, this exercises a path through the waitFor code that
does not even look
at the timeout value so I'll remove the entire 2nd case.

Webrev updated in place.
  http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


Okay, but now you don't need the p.waitFor() after the destroy() at all.

Hmmm ... I guess either we want the original code with a longer
timeout to accommodate slow/loaded platforms (to test that waitFor(n)
after a destroy() is timely); or everything after destroy() can be
deleted.

Here's another approach to test whether waitFor(timeout) is behaving as
expected.
The test should be checking that waitFor does not wait longer than
requested.

So shorten the timeout to a very short time (10 milliseconds) and check that
waitFor returns in no more than that time(plus epsilon).  It is similar
to the first test
in that section but the process is exiting.

   http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


But the first test is checking that we wait at-least as long as the 
timeout ie no early return when we know the process is still alive.


Sorry but this isn't making sense to me. The original problem is that 
waitFor(8) has timed out and we proceeded to call exitValue() while the 
process is still alive. If the process had in fact terminated then we 
would have hit the (end-start) check and failed due to a timeout. So 
what exactly are we trying to fix here? Do we simply prefer to fail due 
to timeout rather than have exitValue() throw IllegalStateException? If 
so then check for waitFor(8) timing out, but leave the timeout value at 
8 seconds - just avoid the call to exitValue() if a time-out occurred. 
Otherwise if we want to avoid timing out we either bump the timeout to 
some value > 8 to accommodate the load problem, or we don't even bother 
doing waitFor after the destroy().


I'm really unclear what this is trying to test, and what a failure would 
indicate.


Thanks,
David


Thanks, Roger




Cheers,
David



Thanks, Roger

David



Roger



David


I contemplated increasing the timeout but given the issue is system
loading
I didn't have a good idea what to raise it to.  30sec, 1 min, 2 min.
and the harness timeout is 2min.

Roger



David


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger

Re: RFR 9: 8168517 : java/lang/ProcessBuilder/Basic.java failed


Hi David,

On 10/27/2016 9:00 PM, David Holmes wrote:

But this is the second waitFor call after the process is destroyed.
Sorry I don't really see the point.

The tests were added to determine if waitFor(timeout) was handling the
timeout parameter correctly.
The 2nd test here was to check the datapath when the process already
been destroyed.
The 'extra' waitFor() ensures the process is gone.
But you are correct, this exercises a path through the waitFor code that
does not even look
at the timeout value so I'll remove the entire 2nd case.

Webrev updated in place.
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/


Okay, but now you don't need the p.waitFor() after the destroy() at all.

Hmmm ... I guess either we want the original code with a longer 
timeout to accommodate slow/loaded platforms (to test that waitFor(n) 
after a destroy() is timely); or everything after destroy() can be 
deleted.
Here's another approach to test whether waitFor(timeout) is behaving as 
expected.
The test should be checking that waitFor does not wait longer than 
requested.


So shorten the timeout to a very short time (10 milliseconds) and check that
waitFor returns in no more than that time(plus epsilon).  It is similar 
to the first test

in that section but the process is exiting.

   http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger




Cheers,
David



Thanks, Roger

David



Roger



David


I contemplated increasing the timeout but given the issue is system
loading
I didn't have a good idea what to raise it to.  30sec, 1 min, 2 min.
and the harness timeout is 2min.

Roger



David


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-destroy-8168517/

Thanks, Roger

Re: RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError


Hi Daniel,

The test is fine.
Without the fix, jtreg is handling the uncaught LinkageError exception
and has called System.exit which will deadlock since it is already in 
System.exit.


Roger


On 10/28/2016 10:44 AM, Daniel Fuchs wrote:

On 28/10/16 14:50, Roger Riggs wrote:

Hi Daniel,

Looks fine.

Roger


Thanks Roger.

I have turned the reproducer attached to JDK-8152515 into a
jtreg test. The test creates some logger and registers a handler
that throws LinkageError when closed.

The test passes as expected with the fix.
Without the fix however, it makes jtreg fail in timeout
(which is a bit strange):

--System.out:(2/193)--
[LinkageErrorTest$TestHandler@3a7f70{closed=false}, 
LinkageErrorTest$TestHandler@25da7d46{closed=false}, 
LinkageErrorTest$TestHandler@6c57d74f{closed=true}]

Timeout signalled after 120 seconds
--System.err:(9/621)--
STATUS:Passed.
java.lang.LinkageError
at LinkageErrorTest$TestHandler.close(LinkageErrorTest.java:63)
at 
java.util.logging.LogManager.closeHandlers(java.logging@9-internal/LogManager.java:1449)
at 
java.util.logging.LogManager.resetLogger(java.logging@9-internal/LogManager.java:1459)
at 
java.util.logging.LogManager.resetLoggerContext(java.logging@9-internal/LogManager.java:1439)
at 
java.util.logging.LogManager.reset(java.logging@9-internal/LogManager.java:1424)
at 
java.util.logging.LogManager$Cleaner.run(java.logging@9-internal/LogManager.java:284)

STATUS:Failed.`main' threw exception: java.lang.LinkageError
--rerun:(21/1847)*--

I am not sure whether I should push this test or not...

What do you think? Is that behavior going to stay stable?
I mean - can we rely on the fact that jtreg will always
fail this test if the fix is not present?

Here is the webrev with the test:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.01

-- daniel




On 10/28/2016 7:51 AM, Daniel Fuchs wrote:

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: RFR: JDK-8167618: DateTimeFormatter.format() uses exceptions for flow control.


+1,

Roger


On 10/28/2016 11:58 AM, nadeesh tv wrote:


Hi Anubhav,


- * @throws DateTimeException if the field is not available and 
the section is not optional



I think you should not remove the DTException since still there is a 
chance of throwing DTE

Regards,
Nadeesh
On 10/28/2016 12:18 AM, Anubhav Meena wrote:

Hi all,

Please review. Please ignore last mail as links not working properly 
there.


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


Issue:  DateTimeFormatter.format() uses exceptions for flow control.
Webrev: 
http://cr.openjdk.java.net/~rchamyal/anmeena/8167618/webrev.00/

Re: 8152515: (logging) LogManager.resetLogger should ignore LinkageError

2016-10-28 Thread Jonathan Bluett-Duncan

Oh, I see! Thanks for pointing out my misconception for me. :)

In that case, this fix looks fine to me as a non-reviewer.

Kind regards,
Jonathan

On 28 October 2016 at 17:15, Roger Riggs  wrote:

> Hi Jonathan,
>
> There is no issue in this case.
> LinkageError does not extend Exception so they are disjoint at the catch
> clauses.
> And the compiler produces an error if a catch clause hides another
> exception to keep
> the mistake from being hidden.
>
> $.02, Roger
>
>
>
> On 10/28/2016 12:00 PM, Jonathan Bluett-Duncan wrote:
>
>> I've an awful suspicion that the `catch (LinkageError e)` block is
>> unreachable, as the `catch (Exception e)` block would run first, being
>> located above the other block in the source code.
>>
>> Is my suspicion correct?
>>
>> Kind regards,
>> Jonathan
>>
>> On 28 October 2016 at 16:36, Jason Mehrens 
>> wrote:
>>
>> Daniel,
>>>
>>> Looks good to me.
>>>
>>> Thanks for fixing this!
>>>
>>> Jason
>>>
>>> 
>>> From: Daniel Fuchs 
>>> Sent: Friday, October 28, 2016 6:51 AM
>>> To: core-libs-dev
>>> Cc: Jason Mehrens
>>> Subject: RFR: 8152515: (logging) LogManager.resetLogger should ignore
>>> LinkageError
>>>
>>> Hi,
>>>
>>> Please find below a trivial  patch for:
>>>
>>> 8152515: (logging) LogManager.resetLogger should ignore LinkageError
>>> https://bugs.openjdk.java.net/browse/JDK-8152515
>>>
>>>
>>> Patch:
>>> http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/
>>>
>>> The issue might occur at shutdown, when a handler that makes uses
>>> of some APIs provided by an OSGI bundle which was already closed
>>> by the shutdown process is in turn closed by the LogManager.Cleaner
>>> thread. In that case some subclasses of LinkageError may be thrown,
>>> interrupting the reset process and preventing other handlers from
>>> being closed properly.
>>>
>>> The patch proposes to trivially ignore LinkageError at shutdown while
>>> the LogManager.Cleaner thread is running.
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>
>

Re: 8152515: (logging) LogManager.resetLogger should ignore LinkageError


Hi Jonathan,

There is no issue in this case.
LinkageError does not extend Exception so they are disjoint at the catch 
clauses.
And the compiler produces an error if a catch clause hides another 
exception to keep

the mistake from being hidden.

$.02, Roger


On 10/28/2016 12:00 PM, Jonathan Bluett-Duncan wrote:

I've an awful suspicion that the `catch (LinkageError e)` block is
unreachable, as the `catch (Exception e)` block would run first, being
located above the other block in the source code.

Is my suspicion correct?

Kind regards,
Jonathan

On 28 October 2016 at 16:36, Jason Mehrens 
wrote:


Daniel,

Looks good to me.

Thanks for fixing this!

Jason


From: Daniel Fuchs 
Sent: Friday, October 28, 2016 6:51 AM
To: core-libs-dev
Cc: Jason Mehrens
Subject: RFR: 8152515: (logging) LogManager.resetLogger should ignore
LinkageError

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: 8152515: (logging) LogManager.resetLogger should ignore LinkageError


Hi Jonathan,

On 28/10/16 17:00, Jonathan Bluett-Duncan wrote:

I've an awful suspicion that the `catch (LinkageError e)` block is
unreachable, as the `catch (Exception e)` block would run first, being
located above the other block in the source code.

Is my suspicion correct?


Not really. As its name indicate, LinkageError is an Error, not a
subclass of Exception.

best regards,

-- daniel



Kind regards,
Jonathan

On 28 October 2016 at 16:36, Jason Mehrens > wrote:

Daniel,

Looks good to me.

Thanks for fixing this!

Jason


From: Daniel Fuchs >
Sent: Friday, October 28, 2016 6:51 AM
To: core-libs-dev
Cc: Jason Mehrens
Subject: RFR: 8152515: (logging) LogManager.resetLogger should
ignore LinkageError

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515



Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/


The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: 8152515: (logging) LogManager.resetLogger should ignore LinkageError

2016-10-28 Thread Jonathan Bluett-Duncan

I've an awful suspicion that the `catch (LinkageError e)` block is
unreachable, as the `catch (Exception e)` block would run first, being
located above the other block in the source code.

Is my suspicion correct?

Kind regards,
Jonathan

On 28 October 2016 at 16:36, Jason Mehrens 
wrote:

> Daniel,
>
> Looks good to me.
>
> Thanks for fixing this!
>
> Jason
>
> 
> From: Daniel Fuchs 
> Sent: Friday, October 28, 2016 6:51 AM
> To: core-libs-dev
> Cc: Jason Mehrens
> Subject: RFR: 8152515: (logging) LogManager.resetLogger should ignore
> LinkageError
>
> Hi,
>
> Please find below a trivial  patch for:
>
> 8152515: (logging) LogManager.resetLogger should ignore LinkageError
> https://bugs.openjdk.java.net/browse/JDK-8152515
>
>
> Patch:
> http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/
>
> The issue might occur at shutdown, when a handler that makes uses
> of some APIs provided by an OSGI bundle which was already closed
> by the shutdown process is in turn closed by the LogManager.Cleaner
> thread. In that case some subclasses of LinkageError may be thrown,
> interrupting the reset process and preventing other handlers from
> being closed properly.
>
> The patch proposes to trivially ignore LinkageError at shutdown while
> the LogManager.Cleaner thread is running.
>
> best regards,
>
> -- daniel
>

Re: RFR: JDK-8167618: DateTimeFormatter.format() uses exceptions for flow control.

2016-10-28 Thread nadeesh tv



Hi Anubhav,


- * @throws DateTimeException if the field is not available and the section 
is not optional


I think you should not remove the DTException since still there is a 
chance of throwing DTE

Regards,
Nadeesh
On 10/28/2016 12:18 AM, Anubhav Meena wrote:

Hi all,

Please review. Please ignore last mail as links not working properly there.

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

Issue:  DateTimeFormatter.format() uses exceptions for flow control.
Webrev: http://cr.openjdk.java.net/~rchamyal/anmeena/8167618/webrev.00/ 



--
Thanks and Regards,
Nadeesh TV

Re: Review Request: JDK-8167057 jdeps to list the modules and internal APIs to help find @modules for tests

2016-10-28 Thread Mandy Chung


> On Oct 28, 2016, at 3:03 AM, Daniel Fuchs  wrote:
> 
> Hi Mandy,
> 
> Looks good to me in general, but I feel like the new
> option --list-reduced-deps should be better documented:
> 

I agree and this details and example can be covered in the man page.

> jdeps.properties:
> 
> 152 main.opt.list-deps=\
> 153 \  --list-deps\n\
> 154 \  --list-reduced-deps   Lists the dependences and use of JDK 
> internal\n\
> 155 \APIs. --list-reduced-deps lists the 
> dependences\n\
> 156 \after transition reduction.
> 
> #1 - is it 'transition reduction' or 'transitive reduction'?
> (at two places in this file: line 156 & line 135)
> 

Good catch.  typo should be “transitive” and will fix it.
  
> #2 - could the description be made a little more verbose? something
> like:
> 
> --list-reduced-deps   lists the dependences
>  after transitive reduction.
>  Transitive reduction is obtained
>  by removing the dependencies which
>  are already transitively exported
>  by another module in the
>  dependency graph. For instance,
>  if both java.sql and java.logging
>  are included in the dependency
>  graph, then java.logging will be
>  removed because it is already
>  transitively exported by
>  java.sql, and therefore
>  requiring java.sql should be
>  enough.

Good idea.  I will expand the description.

Mandy

> 
> 
> best regards,
> 
> -- daniel
> 
> 
> On 19/10/16 23:19, Mandy Chung wrote:
>> Webrev at:
>>  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8167057/webrev.00/
>> 
>> This patch enhances jdeps to print the dependences in the format : 
>> $MODULE[/$PACKAGE].
>> 
>> This is intended for analyzing the regression tests we develop and add make 
>> it easy to add the proper @modules.
>> 
>> Mandy
>> 
>

Re: 8152515: (logging) LogManager.resetLogger should ignore LinkageError

2016-10-28 Thread Jason Mehrens

Daniel,

Looks good to me.

Thanks for fixing this!

Jason


From: Daniel Fuchs 
Sent: Friday, October 28, 2016 6:51 AM
To: core-libs-dev
Cc: Jason Mehrens
Subject: RFR: 8152515: (logging) LogManager.resetLogger should ignore 
LinkageError

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError


On 28/10/16 14:50, Roger Riggs wrote:

Hi Daniel,

Looks fine.

Roger


Thanks Roger.

I have turned the reproducer attached to JDK-8152515 into a
jtreg test. The test creates some logger and registers a handler
that throws LinkageError when closed.

The test passes as expected with the fix.
Without the fix however, it makes jtreg fail in timeout
(which is a bit strange):

--System.out:(2/193)--
[LinkageErrorTest$TestHandler@3a7f70{closed=false}, 
LinkageErrorTest$TestHandler@25da7d46{closed=false}, 
LinkageErrorTest$TestHandler@6c57d74f{closed=true}]

Timeout signalled after 120 seconds
--System.err:(9/621)--
STATUS:Passed.
java.lang.LinkageError
at LinkageErrorTest$TestHandler.close(LinkageErrorTest.java:63)
	at 
java.util.logging.LogManager.closeHandlers(java.logging@9-internal/LogManager.java:1449)
	at 
java.util.logging.LogManager.resetLogger(java.logging@9-internal/LogManager.java:1459)
	at 
java.util.logging.LogManager.resetLoggerContext(java.logging@9-internal/LogManager.java:1439)
	at 
java.util.logging.LogManager.reset(java.logging@9-internal/LogManager.java:1424)
	at 
java.util.logging.LogManager$Cleaner.run(java.logging@9-internal/LogManager.java:284)

STATUS:Failed.`main' threw exception: java.lang.LinkageError
--rerun:(21/1847)*--

I am not sure whether I should push this test or not...

What do you think? Is that behavior going to stay stable?
I mean - can we rely on the fact that jtreg will always
fail this test if the fix is not present?

Here is the webrev with the test:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.01

-- daniel




On 10/28/2016 7:51 AM, Daniel Fuchs wrote:

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: JDK 9 RFR of JDK-8168524: Remove two jdk_nio tests from ProblemList: BashStreams and DeleteInterference.java


+1
(for BashStreams test)

Since this is restoring tests to gather more data.

Roger


On 10/27/2016 11:15 PM, Amy Lu wrote:

On 10/24/16 10:50 PM, Brian Burkhalter wrote:

+1 for the DeleteInterference portion.

Thank you Brian.

Still need a reviewer for BashStreams test.

Thanks,
Amy



Brian

On Oct 24, 2016, at 1:08 AM, Amy Lu  wrote:


Please review the patch to bring back two tests:

java/nio/file/WatchService/DeleteInterference.java 8156511 linux-all
More debug info has been added to this test (JDK-8162902). It's 
hard to reproduce the failure. Let's bring back this test. Once test 
failure happen again, the debug info could help with JDK-8156511 fix.


java/nio/charset/coders/BashStreams.java 8149712 generic-all
Mentioned issue seems does not exist anymore, suggest to bring 
back the test.


bug: https://bugs.openjdk.java.net/browse/JDK-8168524

Thanks,
Amy

--- old/test/ProblemList.txt2016-10-24 15:50:14.0 +0800
+++ new/test/ProblemList.txt2016-10-24 15:50:14.0 +0800
@@ -186,10 +186,6 @@
java/nio/file/WatchService/MayFlies.java 7158947 solaris-all Solaris 11
java/nio/file/WatchService/LotsOfEvents.java 7158947 solaris-all 
Solaris 11


-java/nio/charset/coders/BashStreams.java 8149712 generic-all
-
-java/nio/file/WatchService/DeleteInterference.java 8156511 linux-all
-
 



# jdk_rmi

Re: RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError


Hi Daniel,

Looks fine.

Roger


On 10/28/2016 7:51 AM, Daniel Fuchs wrote:

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError


Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: Review Request: JDK-8167057 jdeps to list the modules and internal APIs to help find @modules for tests