Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Igor Ignatyev]

Hi Chris,

the changeset looks reasonable, reviewed.

Thanks,
-- Igor

> On Mar 21, 2018, at 3:26 PM, David Holmes  wrote:
> 
> Sorry Chris I just don't have time to try and figure this one out. If it 
> works uses it.
> 
> David
> 
> On 22/03/2018 4:24 AM, Chris Plummer wrote:
>> Yeah, this was all new to me. Before this I didn't know anything about jtreg 
>> IO other than the use of OutputAnalyzer for capture and verification.
>> Thanks for reviewing.
>> Chris
>> On 3/21/18 11:08 AM, serguei.spit...@oracle.com wrote:
>>> Hi Chris,
>>> 
>>> It looks good to me.
>>> It is a little bit more complicated than one would expect but reasonable.
>>> 
>>> Thanks,
>>> Serguei
>>> 
>>> 
>>> On 3/21/18 09:31, Chris Plummer wrote:
 Ping. I still need a couple of reviews for this.
 
 thanks,
 
 Chris
 
 On 3/19/18 3:50 PM, Chris Plummer wrote:
> I looked into modifying OutputAnalyzer (actually ended up being 
> ProcessTools that needed all the changes) to be more flexible so it could 
> support LingeredApp. The problem I ran into is that ProcessTools is all 
> static, but I needed to create and return a context. It ended up being 
> too much disruption, so I instead have the ProcessTools.getOutput() code 
> as part of LingeredApp.
> 
> Another thing I discovered is that you can use OutputAnalyzer with 
> already generated output, so this option is still available to users of 
> LingeredApp. You just need to do something like:
> 
> OutputAnalyzer out = new 
> OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
> lingeredApp.getOutput().getStderr());
> 
> I didn't change any test to take advantage of this, but it's there if 
> someone wants it.
> 
> I've included another webrev below (completely different from the 
> original). In the end, all LingeredApp stdout and stderr is dumped after 
> the app exits. The old way of storing away the stdout using an 
> InputGobbler is gone. Since getAppOutput() depended on this, and the new 
> way of saving stdout saves it as one big string rather than a List of 
> lines, getAppOutput() needed some changes to convert to the List form.
> 
> http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03
> 
> thanks,
> 
> Chris
> 
> On 3/19/18 9:39 AM, Chris Plummer wrote:
>> Hi David,
>> 
>> Just to clarify one point, most of the tests that use OutputAnalyzer do 
>> not display process output unless there is an error. So part of the 
>> decision here with LingeredApp is when to display the output. Currently 
>> the stdout is captured, but not displayed, unless the tests does the 
>> work to display it, which none do. Currently stderr goes to the console. 
>> Note that some negative tests actually cause some expected stderr 
>> output, although the tests don't check for it.
>> 
>> One thought I just had is to create an async option for OutputAnalyzer 
>> so it doesn't block until the process exits. Basically that means 
>> splitting ProcessTools.getOutput() so it doesn't block. What I currently 
>> have is essentially doing that. It copies ProcessTools.getOutput(), 
>> splitting it into two parts. But all this logic is in LingeredApp, and 
>> of course doesn't have any of the output error checking support that 
>> OutputAnalyzer, which might be useful for LingeredApp. For example, the 
>> negative tests only test that launching the app failed. They could be 
>> improved by checking for specific error output.
>> 
>> Chris
>> 
>> On 3/17/18 12:11 AM, David Holmes wrote:
>>> I'm afraid I'm losing track of this change.
>>> 
>>> The key thing is that we should not have a test that launches any other 
>>> process for which we can not see the output of that process.
>>> 
>>> David
>>> 
>>> On 17/03/2018 7:48 AM, Chris Plummer wrote:
 On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:
> Hi Chris,
> 
> Thank you for taking care about this issue!
> 
> On 3/16/18 11:20, Chris Plummer wrote:
>> Hi,
>> 
>> I've resolved the issues I had before with not seeing all the stderr 
>> output when I tried to capture it. What I'd like to do now is have 
>> us decide how the output should be handled from the perspective a 
>> LingeredApp user (driver app). Currently all LingeredApp stdout is 
>> captured and gets be returned the the driver app by calling 
>> app.getAppOutput(). It does not appear in the .jtr file, but the 
>> test would have the option of dumping it there it it cared to. Only 
>> one test uses app.getAppOutput(). Currently all the LingeredApp 
>> stderr is redirected to the console, so it does not appear in the 
>> .jtr file.
> 
> Just a general comment to mak

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[David Holmes]

Sorry Chris I just don't have time to try and figure this one out. If it 
works uses it.


David

On 22/03/2018 4:24 AM, Chris Plummer wrote:
Yeah, this was all new to me. Before this I didn't know anything about 
jtreg IO other than the use of OutputAnalyzer for capture and verification.


Thanks for reviewing.

Chris

On 3/21/18 11:08 AM, serguei.spit...@oracle.com wrote:

Hi Chris,

It looks good to me.
It is a little bit more complicated than one would expect but reasonable.

Thanks,
Serguei


On 3/21/18 09:31, Chris Plummer wrote:

Ping. I still need a couple of reviews for this.

thanks,

Chris

On 3/19/18 3:50 PM, Chris Plummer wrote:
I looked into modifying OutputAnalyzer (actually ended up being 
ProcessTools that needed all the changes) to be more flexible so it 
could support LingeredApp. The problem I ran into is that 
ProcessTools is all static, but I needed to create and return a 
context. It ended up being too much disruption, so I instead have 
the ProcessTools.getOutput() code as part of LingeredApp.


Another thing I discovered is that you can use OutputAnalyzer with 
already generated output, so this option is still available to users 
of LingeredApp. You just need to do something like:


    OutputAnalyzer out = new 
OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
lingeredApp.getOutput().getStderr());


I didn't change any test to take advantage of this, but it's there 
if someone wants it.


I've included another webrev below (completely different from the 
original). In the end, all LingeredApp stdout and stderr is dumped 
after the app exits. The old way of storing away the stdout using an 
InputGobbler is gone. Since getAppOutput() depended on this, and the 
new way of saving stdout saves it as one big string rather than a 
List of lines, getAppOutput() needed some changes to convert to the 
List form.


http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03

thanks,

Chris

On 3/19/18 9:39 AM, Chris Plummer wrote:

Hi David,

Just to clarify one point, most of the tests that use 
OutputAnalyzer do not display process output unless there is an 
error. So part of the decision here with LingeredApp is when to 
display the output. Currently the stdout is captured, but not 
displayed, unless the tests does the work to display it, which none 
do. Currently stderr goes to the console. Note that some negative 
tests actually cause some expected stderr output, although the 
tests don't check for it.


One thought I just had is to create an async option for 
OutputAnalyzer so it doesn't block until the process exits. 
Basically that means splitting ProcessTools.getOutput() so it 
doesn't block. What I currently have is essentially doing that. It 
copies ProcessTools.getOutput(), splitting it into two parts. But 
all this logic is in LingeredApp, and of course doesn't have any of 
the output error checking support that OutputAnalyzer, which might 
be useful for LingeredApp. For example, the negative tests only 
test that launching the app failed. They could be improved by 
checking for specific error output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do 
now is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the 
driver app by calling app.getAppOutput(). It does not appear in 
the .jtr file, but the test would have the option of dumping it 
there it it cared to. Only one test uses app.getAppOutput(). 
Currently all the LingeredApp stderr is redirected to the 
console, so it does not appear in the .jtr file.


Just a general comment to make sure I understand it and ensure 
we are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of 
of what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is 
up the the driver app to decide if it wants to display it 
(after the app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fi

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

Yeah, this was all new to me. Before this I didn't know anything about 
jtreg IO other than the use of OutputAnalyzer for capture and verification.


Thanks for reviewing.

Chris

On 3/21/18 11:08 AM, serguei.spit...@oracle.com wrote:

Hi Chris,

It looks good to me.
It is a little bit more complicated than one would expect but reasonable.

Thanks,
Serguei


On 3/21/18 09:31, Chris Plummer wrote:

Ping. I still need a couple of reviews for this.

thanks,

Chris

On 3/19/18 3:50 PM, Chris Plummer wrote:
I looked into modifying OutputAnalyzer (actually ended up being 
ProcessTools that needed all the changes) to be more flexible so it 
could support LingeredApp. The problem I ran into is that 
ProcessTools is all static, but I needed to create and return a 
context. It ended up being too much disruption, so I instead have 
the ProcessTools.getOutput() code as part of LingeredApp.


Another thing I discovered is that you can use OutputAnalyzer with 
already generated output, so this option is still available to users 
of LingeredApp. You just need to do something like:


    OutputAnalyzer out = new 
OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
lingeredApp.getOutput().getStderr());


I didn't change any test to take advantage of this, but it's there 
if someone wants it.


I've included another webrev below (completely different from the 
original). In the end, all LingeredApp stdout and stderr is dumped 
after the app exits. The old way of storing away the stdout using an 
InputGobbler is gone. Since getAppOutput() depended on this, and the 
new way of saving stdout saves it as one big string rather than a 
List of lines, getAppOutput() needed some changes to convert to the 
List form.


http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03

thanks,

Chris

On 3/19/18 9:39 AM, Chris Plummer wrote:

Hi David,

Just to clarify one point, most of the tests that use 
OutputAnalyzer do not display process output unless there is an 
error. So part of the decision here with LingeredApp is when to 
display the output. Currently the stdout is captured, but not 
displayed, unless the tests does the work to display it, which none 
do. Currently stderr goes to the console. Note that some negative 
tests actually cause some expected stderr output, although the 
tests don't check for it.


One thought I just had is to create an async option for 
OutputAnalyzer so it doesn't block until the process exits. 
Basically that means splitting ProcessTools.getOutput() so it 
doesn't block. What I currently have is essentially doing that. It 
copies ProcessTools.getOutput(), splitting it into two parts. But 
all this logic is in LingeredApp, and of course doesn't have any of 
the output error checking support that OutputAnalyzer, which might 
be useful for LingeredApp. For example, the negative tests only 
test that launching the app failed. They could be improved by 
checking for specific error output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do 
now is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the 
driver app by calling app.getAppOutput(). It does not appear in 
the .jtr file, but the test would have the option of dumping it 
there it it cared to. Only one test uses app.getAppOutput(). 
Currently all the LingeredApp stderr is redirected to the 
console, so it does not appear in the .jtr file.


Just a general comment to make sure I understand it and ensure 
we are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of 
of what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is 
up the the driver app to decide if it wants to display it 
(after the app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single 
String instead of a List of Strings, so this breaks the one test 
that uses t

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

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

Hi Chris,

It looks good to me.
It is a little bit more complicated than one would expect but reasonable.

Thanks,
Serguei


On 3/21/18 09:31, Chris Plummer wrote:

Ping. I still need a couple of reviews for this.

thanks,

Chris

On 3/19/18 3:50 PM, Chris Plummer wrote:
I looked into modifying OutputAnalyzer (actually ended up being 
ProcessTools that needed all the changes) to be more flexible so it 
could support LingeredApp. The problem I ran into is that 
ProcessTools is all static, but I needed to create and return a 
context. It ended up being too much disruption, so I instead have the 
ProcessTools.getOutput() code as part of LingeredApp.


Another thing I discovered is that you can use OutputAnalyzer with 
already generated output, so this option is still available to users 
of LingeredApp. You just need to do something like:


    OutputAnalyzer out = new 
OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
lingeredApp.getOutput().getStderr());


I didn't change any test to take advantage of this, but it's there if 
someone wants it.


I've included another webrev below (completely different from the 
original). In the end, all LingeredApp stdout and stderr is dumped 
after the app exits. The old way of storing away the stdout using an 
InputGobbler is gone. Since getAppOutput() depended on this, and the 
new way of saving stdout saves it as one big string rather than a 
List of lines, getAppOutput() needed some changes to convert to the 
List form.


http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03

thanks,

Chris

On 3/19/18 9:39 AM, Chris Plummer wrote:

Hi David,

Just to clarify one point, most of the tests that use OutputAnalyzer 
do not display process output unless there is an error. So part of 
the decision here with LingeredApp is when to display the output. 
Currently the stdout is captured, but not displayed, unless the 
tests does the work to display it, which none do. Currently stderr 
goes to the console. Note that some negative tests actually cause 
some expected stderr output, although the tests don't check for it.


One thought I just had is to create an async option for 
OutputAnalyzer so it doesn't block until the process exits. 
Basically that means splitting ProcessTools.getOutput() so it 
doesn't block. What I currently have is essentially doing that. It 
copies ProcessTools.getOutput(), splitting it into two parts. But 
all this logic is in LingeredApp, and of course doesn't have any of 
the output error checking support that OutputAnalyzer, which might 
be useful for LingeredApp. For example, the negative tests only test 
that launching the app failed. They could be improved by checking 
for specific error output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do 
now is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the 
driver app by calling app.getAppOutput(). It does not appear in 
the .jtr file, but the test would have the option of dumping it 
there it it cared to. Only one test uses app.getAppOutput(). 
Currently all the LingeredApp stderr is redirected to the 
console, so it does not appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we 
are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of 
of what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is 
up the the driver app to decide if it wants to display it (after 
the app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single 
String instead of a List of Strings, so this breaks the one test 
that uses this API. It's easily fixed. Just haven't gotten around 
to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported).

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

Ping. I still need a couple of reviews for this.

thanks,

Chris

On 3/19/18 3:50 PM, Chris Plummer wrote:
I looked into modifying OutputAnalyzer (actually ended up being 
ProcessTools that needed all the changes) to be more flexible so it 
could support LingeredApp. The problem I ran into is that ProcessTools 
is all static, but I needed to create and return a context. It ended 
up being too much disruption, so I instead have the 
ProcessTools.getOutput() code as part of LingeredApp.


Another thing I discovered is that you can use OutputAnalyzer with 
already generated output, so this option is still available to users 
of LingeredApp. You just need to do something like:


    OutputAnalyzer out = new 
OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
lingeredApp.getOutput().getStderr());


I didn't change any test to take advantage of this, but it's there if 
someone wants it.


I've included another webrev below (completely different from the 
original). In the end, all LingeredApp stdout and stderr is dumped 
after the app exits. The old way of storing away the stdout using an 
InputGobbler is gone. Since getAppOutput() depended on this, and the 
new way of saving stdout saves it as one big string rather than a List 
of lines, getAppOutput() needed some changes to convert to the List form.


http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03

thanks,

Chris

On 3/19/18 9:39 AM, Chris Plummer wrote:

Hi David,

Just to clarify one point, most of the tests that use OutputAnalyzer 
do not display process output unless there is an error. So part of 
the decision here with LingeredApp is when to display the output. 
Currently the stdout is captured, but not displayed, unless the tests 
does the work to display it, which none do. Currently stderr goes to 
the console. Note that some negative tests actually cause some 
expected stderr output, although the tests don't check for it.


One thought I just had is to create an async option for 
OutputAnalyzer so it doesn't block until the process exits. Basically 
that means splitting ProcessTools.getOutput() so it doesn't block. 
What I currently have is essentially doing that. It copies 
ProcessTools.getOutput(), splitting it into two parts. But all this 
logic is in LingeredApp, and of course doesn't have any of the output 
error checking support that OutputAnalyzer, which might be useful for 
LingeredApp. For example, the negative tests only test that launching 
the app failed. They could be improved by checking for specific error 
output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do now 
is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the 
driver app by calling app.getAppOutput(). It does not appear in 
the .jtr file, but the test would have the option of dumping it 
there it it cared to. Only one test uses app.getAppOutput(). 
Currently all the LingeredApp stderr is redirected to the 
console, so it does not appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we 
are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of of 
what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up 
the the driver app to decide if it wants to display it (after the 
app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single String 
instead of a List of Strings, so this breaks the one test that uses 
this API. It's easily fixed. Just haven't gotten around to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this 
is to keep the two streams separated instead of sending both to 
stdout.


Let me know what you think. I'm inclined to go with 2, especia

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

I looked into modifying OutputAnalyzer (actually ended up being 
ProcessTools that needed all the changes) to be more flexible so it 
could support LingeredApp. The problem I ran into is that ProcessTools 
is all static, but I needed to create and return a context. It ended up 
being too much disruption, so I instead have the 
ProcessTools.getOutput() code as part of LingeredApp.


Another thing I discovered is that you can use OutputAnalyzer with 
already generated output, so this option is still available to users of 
LingeredApp. You just need to do something like:


    OutputAnalyzer out = new 
OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
lingeredApp.getOutput().getStderr());


I didn't change any test to take advantage of this, but it's there if 
someone wants it.


I've included another webrev below (completely different from the 
original). In the end, all LingeredApp stdout and stderr is dumped after 
the app exits. The old way of storing away the stdout using an 
InputGobbler is gone. Since getAppOutput() depended on this, and the new 
way of saving stdout saves it as one big string rather than a List of 
lines, getAppOutput() needed some changes to convert to the List form.


http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03

thanks,

Chris

On 3/19/18 9:39 AM, Chris Plummer wrote:

Hi David,

Just to clarify one point, most of the tests that use OutputAnalyzer 
do not display process output unless there is an error. So part of the 
decision here with LingeredApp is when to display the output. 
Currently the stdout is captured, but not displayed, unless the tests 
does the work to display it, which none do. Currently stderr goes to 
the console. Note that some negative tests actually cause some 
expected stderr output, although the tests don't check for it.


One thought I just had is to create an async option for OutputAnalyzer 
so it doesn't block until the process exits. Basically that means 
splitting ProcessTools.getOutput() so it doesn't block. What I 
currently have is essentially doing that. It copies 
ProcessTools.getOutput(), splitting it into two parts. But all this 
logic is in LingeredApp, and of course doesn't have any of the output 
error checking support that OutputAnalyzer, which might be useful for 
LingeredApp. For example, the negative tests only test that launching 
the app failed. They could be improved by checking for specific error 
output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do now 
is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the driver 
app by calling app.getAppOutput(). It does not appear in the .jtr 
file, but the test would have the option of dumping it there it it 
cared to. Only one test uses app.getAppOutput(). Currently all the 
LingeredApp stderr is redirected to the console, so it does not 
appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we 
are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of of 
what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up 
the the driver app to decide if it wants to display it (after the 
app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single String 
instead of a List of Strings, so this breaks the one test that uses 
this API. It's easily fixed. Just haven't gotten around to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this 
is to keep the two streams separated instead of sending both to 
stdout.


Let me know what you think. I'm inclined to go with 2, especially 
since normally there is little to no output from the LingeredApp.


The choice (2) looks good enough.
N

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

Hi David,

Just to clarify one point, most of the tests that use OutputAnalyzer do 
not display process output unless there is an error. So part of the 
decision here with LingeredApp is when to display the output. Currently 
the stdout is captured, but not displayed, unless the tests does the 
work to display it, which none do. Currently stderr goes to the console. 
Note that some negative tests actually cause some expected stderr 
output, although the tests don't check for it.


One thought I just had is to create an async option for OutputAnalyzer 
so it doesn't block until the process exits. Basically that means 
splitting ProcessTools.getOutput() so it doesn't block. What I currently 
have is essentially doing that. It copies ProcessTools.getOutput(), 
splitting it into two parts. But all this logic is in LingeredApp, and 
of course doesn't have any of the output error checking support that 
OutputAnalyzer, which might be useful for LingeredApp. For example, the 
negative tests only test that launching the app failed. They could be 
improved by checking for specific error output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do now 
is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the driver 
app by calling app.getAppOutput(). It does not appear in the .jtr 
file, but the test would have the option of dumping it there it it 
cared to. Only one test uses app.getAppOutput(). Currently all the 
LingeredApp stderr is redirected to the console, so it does not 
appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we 
are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of of 
what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up 
the the driver app to decide if it wants to display it (after the 
app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single String 
instead of a List of Strings, so this breaks the one test that uses 
this API. It's easily fixed. Just haven't gotten around to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this is 
to keep the two streams separated instead of sending both to stdout.


Let me know what you think. I'm inclined to go with 2, especially 
since normally there is little to no output from the LingeredApp.


The choice (2) looks good enough.
Not sure it is that important to have output from stdout and stderr 
sync'ed
but is is important to have the stderr present in the .jtr 
automatically.


The choice (3) looks even better if it is going to work well.
This is basically what the original webrev did. It sent LingeredApp's 
stderr and stdout to the the driver apps stdout. It's a 1 word change 
to make it send stderr to stderr. I think it has a bug though that 
did not manifest itself. It seems the new copy() code that is 
capturing stdout would be contending with the existing InputGlobbler 
code that is doing the same. I would need to fix this to make sure 
LingeredApp.getAppOutput() still returns all the apps stdout output.


Chris

Not sure, it is really necessary.

Thanks,
Serguei




BTW, here's the CR and original webrev for reference:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[David Holmes]

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any other 
process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the stderr 
output when I tried to capture it. What I'd like to do now is have us 
decide how the output should be handled from the perspective a 
LingeredApp user (driver app). Currently all LingeredApp stdout is 
captured and gets be returned the the driver app by calling 
app.getAppOutput(). It does not appear in the .jtr file, but the test 
would have the option of dumping it there it it cared to. Only one 
test uses app.getAppOutput(). Currently all the LingeredApp stderr is 
redirected to the console, so it does not appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we are 
in sync.
It seems much more safe to always have both stdout and stderr outputs 
present in the .jtr automatically file independently of of what the 
test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up the 
the driver app to decide if it wants to display it (after the app 
terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's stdout 
and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single String 
instead of a List of Strings, so this breaks the one test that uses this 
API. It's easily fixed. Just haven't gotten around to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this is 
to keep the two streams separated instead of sending both to stdout.


Let me know what you think. I'm inclined to go with 2, especially 
since normally there is little to no output from the LingeredApp.


The choice (2) looks good enough.
Not sure it is that important to have output from stdout and stderr 
sync'ed

but is is important to have the stderr present in the .jtr automatically.

The choice (3) looks even better if it is going to work well.
This is basically what the original webrev did. It sent LingeredApp's 
stderr and stdout to the the driver apps stdout. It's a 1 word change to 
make it send stderr to stderr. I think it has a bug though that did not 
manifest itself. It seems the new copy() code that is capturing stdout 
would be contending with the existing InputGlobbler code that is doing 
the same. I would need to fix this to make sure 
LingeredApp.getAppOutput() still returns all the apps stdout output.


Chris

Not sure, it is really necessary.

Thanks,
Serguei




BTW, here's the CR and original webrev for reference:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the stderr 
output when I tried to capture it. What I'd like to do now is have us 
decide how the output should be handled from the perspective a 
LingeredApp user (driver app). Currently all LingeredApp stdout is 
captured and gets be returned the the driver app by calling 
app.getAppOutput(). It does not appear in the .jtr file, but the test 
would have the option of dumping it there it it cared to. Only one 
test uses app.getAppOutput(). Currently all the LingeredApp stderr is 
redirected to the console, so it does not appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we are 
in sync.
It seems much more safe to always have both stdout and stderr outputs 
present in the .jtr automatically file independently of of what the 
test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up the 
the driver app to decide if it wants to display it (after the app 
terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's stdout 
and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single String 
instead of a List of Strings, so this breaks the one test that uses this 
API. It's easily fixed. Just haven't gotten around to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this is 
to keep the two streams separated instead of sending both to stdout.


Let me know what you think. I'm inclined to go with 2, especially 
since normally there is little to no output from the LingeredApp.


The choice (2) looks good enough.
Not sure it is that important to have output from stdout and stderr 
sync'ed

but is is important to have the stderr present in the .jtr automatically.

The choice (3) looks even better if it is going to work well.
This is basically what the original webrev did. It sent LingeredApp's 
stderr and stdout to the the driver apps stdout. It's a 1 word change to 
make it send stderr to stderr. I think it has a bug though that did not 
manifest itself. It seems the new copy() code that is capturing stdout 
would be contending with the existing InputGlobbler code that is doing 
the same. I would need to fix this to make sure 
LingeredApp.getAppOutput() still returns all the apps stdout output.


Chris

Not sure, it is really necessary.

Thanks,
Serguei




BTW, here's the CR and original webrev for reference:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

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

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the stderr 
output when I tried to capture it. What I'd like to do now is have us 
decide how the output should be handled from the perspective a 
LingeredApp user (driver app). Currently all LingeredApp stdout is 
captured and gets be returned the the driver app by calling 
app.getAppOutput(). It does not appear in the .jtr file, but the test 
would have the option of dumping it there it it cared to. Only one 
test uses app.getAppOutput(). Currently all the LingeredApp stderr is 
redirected to the console, so it does not appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we are in 
sync.
It seems much more safe to always have both stdout and stderr outputs 
present in the .jtr automatically file independently of of what the test 
does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up the 
the driver app to decide if it wants to display it (after the app 
terminates).


It does not look good to me (see above) but maybe I'm missing something 
important here.


(2) capture stderr just like stdout currently is, but have LingeredApp 
automatically send captured output to driver app's stdout and stderr 
(after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?


(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this is to 
keep the two streams separated instead of sending both to stdout.


Let me know what you think. I'm inclined to go with 2, especially 
since normally there is little to no output from the LingeredApp.


The choice (2) looks good enough.
Not sure it is that important to have output from stdout and stderr sync'ed
but is is important to have the stderr present in the .jtr automatically.

The choice (3) looks even better if it is going to work well.
Not sure, it is really necessary.

Thanks,
Serguei




BTW, here's the CR and original webrev for reference:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

Hi,

I've resolved the issues I had before with not seeing all the stderr 
output when I tried to capture it. What I'd like to do now is have us 
decide how the output should be handled from the perspective a 
LingeredApp user (driver app). Currently all LingeredApp stdout is 
captured and gets be returned the the driver app by calling 
app.getAppOutput(). It does not appear in the .jtr file, but the test 
would have the option of dumping it there it it cared to. Only one test 
uses app.getAppOutput(). Currently all the LingeredApp stderr is 
redirected to the console, so it does not appear in the .jtr file.


So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up the 
the driver app to decide if it wants to display it (after the app 
terminates).


(2) capture stderr just like stdout currently is, but have LingeredApp 
automatically send captured output to driver app's stdout and stderr 
(after the app terminates).


(3) send the LingeredApp's stdout and stderr to the driver app's stdout 
as it is being captured (this was the original fix Igor suggested and 
the webrev supported). A minor alternative to this is to keep the two 
streams separated instead of sending both to stdout.


Let me know what you think. I'm inclined to go with 2, especially since 
normally there is little to no output from the LingeredApp.


BTW, here's the CR and original webrev for reference:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

On 3/12/18 1:51 PM, Chris Plummer wrote:

Hi Igor,

On 3/12/18 1:26 PM, Igor Ignatyev wrote:



On Mar 12, 2018, at 8:53 AM, Chris Plummer > wrote:


On 3/11/18 7:52 PM, David Holmes wrote:

Hi Chris,

On 10/03/2018 6:46 AM, Chris Plummer wrote:

Hello,

Please help review the following:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

In the end there were two issues. The first was that the 
pb.redirectError() call was redirecting the LingeredApp's stderr 
to the console, which we don't want. The second was that nothing 
was capturing the LingeredApp's output and sending it to the 
driver app's output (jtr file). These changes make all the 
LingeredApp's output end up in the jtr file.


It isn't clear to me how the interleaving of the two streams by the 
two threads is handled in the copy routine. Are we guaranteed to 
get complete lines of output from each stream before writing to 
System.out?

Hi David,

I'm hoping Igor will chime in here, since this is just cloned from 
some closed code he wrote, and he recommended this fix. Perhaps we 
are just doing something a bit non standard here. When spawning a 
separate test process, don't we normally just dump stdout and stderr 
separately via OutputAnalyzer.reportDiagnosticSummary() after the 
test completes, and then only if there is an error. I'm not sure why 
Igor felt LingeredApp tests should be handled differently.


I recommended this fix as one of possibilities and never claimed it's 
the best solution ;)  I don't know much of LingeredApp tests, so I 
just suggested the patch which only solves the problem I noticed 
(LingeredApp's cerr being printed into jtreg agent's cerr).
If by "into jtreg agent's cerr" you are referring to the presence of 
JDWP "ERROR" messages in the jtreg console, that is fixed simply by 
removing the following:


   pb.redirectError(ProcessBuilder.Redirect.INHERIT);

And that is already part of this fix. But it actually makes the ERROR 
messages completely disappear. The copy() part of the fix makes all 
the LingeredApp output appear in the .jtr file (including the JDWP 
"ERROR" messages).
OutputAnalyzer might not fit the use case of LingeredApp b/c it 
blocks till the process is finished. again, I don't know much about 
LingeredApp itself and the tests which use it.
My point was that other jtreg tests that use ProcessBuilder and 
OutputAnalyzer don't print out anything from the spawned process/app 
until it is done, and even then usually only if there was a test 
failure. Why is there a need here to print out messages as they are 
generated.
I played around with OutputAnalyzer a bit to see if we can do something 
more like are other uses of ProcessBuilder. Unfortunately it did not work:


    appProcess = pb.start();
    OutputAnalyzer output = new OutputAnalyzer(appProcess);

The problem is that OutputAnalyzer(appProcess) will not return until 
appProcess exits, but the test requires interaction with appProcess 
before it will exit, and this is done from the same thread. So unless 
appProcess aborts unexpectedly, we block in OutputAnalyzer(appProcess).


So next I tried getting the InputGobbler output by calling 
LingeredApp.getOutput(). This was missing the stderr output. So I 
created a separate gobbler for LingeredApp.getErrorStream(). It only 
contained the following:


 [Debugger failed to attach: ERROR: Peer not allowed to connect: 
127.0.0.1, ]


Yet the console (without disabling the 
pb.redirectError(ProcessBuilder.Redirect.INHERIT) code) normally would 
contain:


[2018-03-14 09:49:38,354] Agent[1]: stderr: Debugger failed to attach: 
ERROR: Peer not allowed to connect: 127.0.0.1

[2018-03-14 09:49:38,355] Agent[1]: stderr:
[2018-03-14 09:49:39,075] Agent[1]: stderr: Error in allow option: 
'127.0.0.1;192.168.0.0/24'
[2018-03-14 09:49:39,075] Agent[1]: stderr: ERROR: transport error 103: 
invalid IP address in allow option
[2018-03-14 09:49:39,075] Agent[1]: stderr: ERROR: JDWP Transport 
dt_socket failed to initialize, TRANSPORT_INIT(510)
[2018-03-14 09:49:39,075] Agent[1]: stderr: JDWP exit error 
AGENT_ERROR_TRANSPORT_INIT(197): No transports initialized [:732]
[2018-03-14 09:49:40,064] Agent[1]: stderr: ERROR: JDWP option syntax 
error: 
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:50103,allow=
[2018-03-14 09:49:41,289] Agent[1]: stderr: Error in allow option: 
'*+allow=127.0.0.1'
[2018-03-14 09:49:41,289] Agent[1]: stderr: ERROR: transport error 103: 
allow option '*' cannot be expanded
[2018-03-14 09:49:41,289] Agent[1]: stderr: ERROR: JDWP Transport 
dt_socket failed to initialize, TRANSPORT_INIT(510)
[2018-03-14 09:49:41,289] Agent[1]: stderr: JDWP exit error 
AGENT_ERROR_TRANSPORT_INIT(197): No transports initialized [:732]
[2018-03-14 09:49:42,418] Agent[1]: stderr: Error in allow option: 
'allow=127.0.0.1+*'
[2018-03-14 09:49:42,418] Agent[1]: stderr: ERROR: transport e

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

Hi Igor,

On 3/12/18 1:26 PM, Igor Ignatyev wrote:



On Mar 12, 2018, at 8:53 AM, Chris Plummer > wrote:


On 3/11/18 7:52 PM, David Holmes wrote:

Hi Chris,

On 10/03/2018 6:46 AM, Chris Plummer wrote:

Hello,

Please help review the following:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

In the end there were two issues. The first was that the 
pb.redirectError() call was redirecting the LingeredApp's stderr to 
the console, which we don't want. The second was that nothing was 
capturing the LingeredApp's output and sending it to the driver 
app's output (jtr file). These changes make all the LingeredApp's 
output end up in the jtr file.


It isn't clear to me how the interleaving of the two streams by the 
two threads is handled in the copy routine. Are we guaranteed to get 
complete lines of output from each stream before writing to System.out?

Hi David,

I'm hoping Igor will chime in here, since this is just cloned from 
some closed code he wrote, and he recommended this fix. Perhaps we 
are just doing something a bit non standard here. When spawning a 
separate test process, don't we normally just dump stdout and stderr 
separately via OutputAnalyzer.reportDiagnosticSummary() after the 
test completes, and then only if there is an error. I'm not sure why 
Igor felt LingeredApp tests should be handled differently.


I recommended this fix as one of possibilities and never claimed it's 
the best solution ;)  I don't know much of LingeredApp tests, so I 
just suggested the patch which only solves the problem I noticed 
(LingeredApp's cerr being printed into jtreg agent's cerr).
If by "into jtreg agent's cerr" you are referring to the presence of 
JDWP "ERROR" messages in the jtreg console, that is fixed simply by 
removing the following:


   pb.redirectError(ProcessBuilder.Redirect.INHERIT);

And that is already part of this fix. But it actually makes the ERROR 
messages completely disappear. The copy() part of the fix makes all the 
LingeredApp output appear in the .jtr file (including the JDWP "ERROR" 
messages).
OutputAnalyzer might not fit the use case of LingeredApp b/c it blocks 
till the process is finished. again, I don't know much about 
LingeredApp itself and the tests which use it.
My point was that other jtreg tests that use ProcessBuilder and 
OutputAnalyzer don't print out anything from the spawned process/app 
until it is done, and even then usually only if there was a test 
failure. Why is there a need here to print out messages as they are 
generated.


answering to David's question, copy routine handles interleaving of 
two streams similarly to printf routine, it does not do that at all. 
we are writing to System.out as we read data, the only guarantee we 
have is all the bytes we read into buffer will be written together 
(which might mean 1 byte at a time), no guarantees about lines. the 
behavior is pretty much the same as you expect to get from an 
interactive shell w/ both cout and cerr are being printed on the console.
Not quite. If a single threaded app is sending to both System.out and 
System.err (and/or stdout and stderr) and does something to ensure 
flushing after each print or each line, then the output should appear 
cleanly in the order executed. By having two different threads read 
these two streams, order might be changed, and possibly even interleaved 
within lines.


cheers,

Chris


Thanks,
-- Igor


thanks,

Chris


Thanks,
David
-


Tested by running all tests that use LingeredApp.

thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Igor Ignatyev]

> On Mar 12, 2018, at 8:53 AM, Chris Plummer  wrote:
> 
> On 3/11/18 7:52 PM, David Holmes wrote:
>> Hi Chris,
>> 
>> On 10/03/2018 6:46 AM, Chris Plummer wrote:
>>> Hello,
>>> 
>>> Please help review the following:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8198655
>>> http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/
>>> 
>>> In the end there were two issues. The first was that the pb.redirectError() 
>>> call was redirecting the LingeredApp's stderr to the console, which we 
>>> don't want. The second was that nothing was capturing the LingeredApp's 
>>> output and sending it to the driver app's output (jtr file). These changes 
>>> make all the LingeredApp's output end up in the jtr file.
>> 
>> It isn't clear to me how the interleaving of the two streams by the two 
>> threads is handled in the copy routine. Are we guaranteed to get complete 
>> lines of output from each stream before writing to System.out?
> Hi David,
> 
> I'm hoping Igor will chime in here, since this is just cloned from some 
> closed code he wrote, and he recommended this fix. Perhaps we are just doing 
> something a bit non standard here. When spawning a separate test process, 
> don't we normally just dump stdout and stderr separately via 
> OutputAnalyzer.reportDiagnosticSummary() after the test completes, and then 
> only if there is an error. I'm not sure why Igor felt LingeredApp tests 
> should be handled differently.

I recommended this fix as one of possibilities and never claimed it's the best 
solution ;)  I don't know much of LingeredApp tests, so I just suggested the 
patch which only solves the problem I noticed (LingeredApp's cerr being printed 
into jtreg agent's cerr). OutputAnalyzer might not fit the use case of 
LingeredApp b/c it blocks till the process is finished. again, I don't know 
much about LingeredApp itself and the tests which use it. 

answering to David's question, copy routine handles interleaving of two streams 
similarly to printf routine, it does not do that at all. we are writing to 
System.out as we read data, the only guarantee we have is all the bytes we read 
into buffer will be written together (which might mean 1 byte at a time), no 
guarantees about lines. the behavior is pretty much the same as you expect to 
get from an interactive shell w/ both cout and cerr are being printed on the 
console.

Thanks,
-- Igor
> 
> thanks,
> 
> Chris
>> 
>> Thanks,
>> David
>> -
>> 
>>> Tested by running all tests that use LingeredApp.
>>> 
>>> thanks,
>>> 
>>> Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

On 3/11/18 7:52 PM, David Holmes wrote:

Hi Chris,

On 10/03/2018 6:46 AM, Chris Plummer wrote:

Hello,

Please help review the following:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

In the end there were two issues. The first was that the 
pb.redirectError() call was redirecting the LingeredApp's stderr to 
the console, which we don't want. The second was that nothing was 
capturing the LingeredApp's output and sending it to the driver app's 
output (jtr file). These changes make all the LingeredApp's output 
end up in the jtr file.


It isn't clear to me how the interleaving of the two streams by the 
two threads is handled in the copy routine. Are we guaranteed to get 
complete lines of output from each stream before writing to System.out?

Hi David,

I'm hoping Igor will chime in here, since this is just cloned from some 
closed code he wrote, and he recommended this fix. Perhaps we are just 
doing something a bit non standard here. When spawning a separate test 
process, don't we normally just dump stdout and stderr separately via 
OutputAnalyzer.reportDiagnosticSummary() after the test completes, and 
then only if there is an error. I'm not sure why Igor felt LingeredApp 
tests should be handled differently.


thanks,

Chris


Thanks,
David
-


Tested by running all tests that use LingeredApp.

thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

On 3/12/18 3:27 AM, Langer, Christoph wrote:

Hi Chris,


Hi Chris,

On 10/03/2018 6:46 AM, Chris Plummer wrote:

Hello,

Please help review the following:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

In the end there were two issues. The first was that the
pb.redirectError() call was redirecting the LingeredApp's stderr to the
console, which we don't want. The second was that nothing was capturing
the LingeredApp's output and sending it to the driver app's output (jtr
file). These changes make all the LingeredApp's output end up in the jtr
file.

It isn't clear to me how the interleaving of the two streams by the two
threads is handled in the copy routine. Are we guaranteed to get
complete lines of output from each stream before writing to System.out?

Would perhaps the use of a BufferedReader in this place be appropriate, using 
readLine()?

Hi Christoph,

That would be an improvement to the interleaving of stderr and stdout, 
although there could still be issues. For example, if the test 
intentionally left out newlines as it built a long line that might take 
a while to construct (think of printing a "." each second or something 
like that). Also, if the last line was missing a newline, it would never 
be printed.


thanks,

Chris


Another small remark: The indentation of line 361" } catch (IOException 
e) {" seems too deep.

Best regards
Christoph

RE: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Langer, Christoph]

Hi Chris,

> Hi Chris,
> 
> On 10/03/2018 6:46 AM, Chris Plummer wrote:
> > Hello,
> >
> > Please help review the following:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8198655
> > http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/
> >
> > In the end there were two issues. The first was that the
> > pb.redirectError() call was redirecting the LingeredApp's stderr to the
> > console, which we don't want. The second was that nothing was capturing
> > the LingeredApp's output and sending it to the driver app's output (jtr
> > file). These changes make all the LingeredApp's output end up in the jtr
> > file.
> 
> It isn't clear to me how the interleaving of the two streams by the two
> threads is handled in the copy routine. Are we guaranteed to get
> complete lines of output from each stream before writing to System.out?

Would perhaps the use of a BufferedReader in this place be appropriate, using 
readLine()?

Another small remark: The indentation of line 361" } catch 
(IOException e) {" seems too deep.

Best regards
Christoph

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[David Holmes]

Hi Chris,

On 10/03/2018 6:46 AM, Chris Plummer wrote:

Hello,

Please help review the following:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

In the end there were two issues. The first was that the 
pb.redirectError() call was redirecting the LingeredApp's stderr to the 
console, which we don't want. The second was that nothing was capturing 
the LingeredApp's output and sending it to the driver app's output (jtr 
file). These changes make all the LingeredApp's output end up in the jtr 
file.


It isn't clear to me how the interleaving of the two streams by the two 
threads is handled in the copy routine. Are we guaranteed to get 
complete lines of output from each stream before writing to System.out?


Thanks,
David
-


Tested by running all tests that use LingeredApp.

thanks,

Chris