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

[Alexey Utkin]

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


On 7/29/2013 2:32 AM, Alan Bateman wrote:
> On 11/07/2013 02:03, Alexey Utkin wrote:
>> :
>>
>> Bug description:
>>      https://jbs.oracle.com/bugs/browse/JDK-7147084
>>      http://bugs.sun.com/view_bug.do?bug_id=7147084
>>
>> Here is the suggested fix:
>> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7147084/webrev.01/
>>
>> Summary for  v1 changes:
>>
>> -- The set of handles that need to restore the inherit flag was extended
>> by child process IOE handles. That was done to avoid "greedy sibling"
>> problem for the file handles. The file handles are closed outside the
>> synchronized block.
>> -- "Greedy sibling" problem is covered by the
>> [test/java/lang/ProcessBuilder/SiblingIOEHandle.java] test.
>> -- The fact that the set of current process standard IOE handles and the
>> set of child process IOE handles can intersect was taken into account.
>> -- The [test/java/lang/ProcessBuilder/InheritIOEHandle.java] was changed
>> in accordance with Martin's concern.
>>
> I've done a first pass over this. This is a long standing issue so a 
> big thank you for taking this on. It's unfortunate that this requires 
> serializing the exec but based on the various Microsoft articles that 
> you cited when researching this one then it seems that this is what we 
> have to do.
>
> Overall I don't see any issues. It's nice to have the create function 
> broken up into smaller functions. I do think the new code needs 
> several comments, particularly initHolder and the constants.
>
> One concern with the tests is that 10 seconds might not be sufficient 
> on a slow/busy machine or a Windows machine that is being choked by AV 
> software. Could this be changed so that it just getting timed by the 
> test harness if the child does not terminate? I'm also wondering about 
> waitAbit and whether this is useful.
>
> -Alan.
>