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 <chris.plummer at oracle.com 
>> <mailto:chris.plummer at oracle.com>> 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
>