Review request: JDK-7147084 (process) appA hangs when read output stream of appB which starts appC that runs forever (v.3)

[Alexey Utkin]

Here is new version of the fix:
http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7147084/webrev.03/

On 8/6/2013 8:48 AM, Alan Bateman wrote:
> On 05/08/2013 09:05, Alexey Utkin wrote:
>> Here is new version of the fix:
>> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7147084/webrev.02/
>>
>> Changes (in accordance with Alan's recommendations):
>> -- Magic constant -1 was changed to JAVA_INVALID_HANDLE_VALUE with 
>> explanation why the macro
>> INVALID_HANDLE_VALUE is not in use.
>> -- The comments were added for each usage of the 
>> JAVA_INVALID_HANDLE_VALUE macro.
>> -- 10 seconds time limit was rewritten to file signals.
>>
>> Regards,
>> -uta
> Thanks for adding comments. I still think initHolder will be confusing 
> to future maintainers and could benefit from a clear comment (above 
> the static BOOL ...) to explain the parameters. One typo in the 
> comment in " [thisProcessEnd] has no the inherit flag.". The other 
> functions are mostly clear now (thanks).
The sentence was changed, more comments were written.

> There are a few formatting oddities in 
> Java_java_lang_ProcessImpl_create. Looks like a newline has crept into 
> the if statement at L337. Then L358-364 is mis-aligned which makes it 
> hard to tie the JNI ReleaseStringChars back to the JNI GetStringChars.
Thanks, that was [hg diff]/[patch] artifacts. Fixed.

> Thanks for replacing the fragile 10s in the tests. If I read performC 
> (in both tests) correctly then the test will still pass after 5 
> minutes even if the JDK is broken - is that right? Just wonder if it 
> would be better to just replace the for loop with while (true) and let 
> the test harness timeout the test.
That is not so easy. The same program body is used 3 times in 3 
different process: "A", "B", and "C". The process "A" life cycle in 
managed by JPRT machinery. "B" is a short-living intermediate process. 
"C" is a long-living "greedy" process. "C" process is unknown for JPRT 
and is living till the signal from process "A", or 5 min if the test 
failed. Hope that is long enough for slow machines.

> I'm also wondering about waitAbit and whether this is useful. 
[waitAbit] is used to force the thread context switching.

>
> Otherwise I think this is good work and we are finally close to the 
> finish line.
>
> -Alan.