RFR [8034262] Test java/lang/ProcessBuilder/CloseRace.java fails

[David Holmes]

Ivan,

Ignore my earlier email as I hadn't seen the follow ups.

You don't need the CountDownLatch - but it is harmless.

Otherwise looks good to me.

Thanks,
David

On 12/03/2014 6:36 PM, Ivan Gerasimov wrote:
> Thanks Martin
>
> On 12.03.2014 2:45, Martin Buchholz wrote:
>> Well done!
>>
>> Not checking for interrupt in the loop is clearly my bug.
>>
>> dumpAllStacks looks like a very useful utility method (basically what
>> jstack does).  It feels like we're reinventing the wheel here, but I'm
>> not sure.
>>
>
> jstack calls the hotspot-specific code to dump the stack traces, but I
> used the jdk-level routine.
> The dumpAllStacks() function has already done its job, so I could
> probably just remove it.
> Though it may be better to keep it just in case.
>
>> Instead of "break theLoop" I would have simply returned from the method.
>>
>
> OK, did that. Here's the updated webrev:
>
> http://cr.openjdk.java.net/~igerasim/8034262/2/webrev/
>
> Sincerely yours,
> Ivan
>
>>
>> On Tue, Mar 11, 2014 at 11:03 AM, Ivan Gerasimov
>> <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>> wrote:
>>
>>     Thank you Martin!
>>
>>
>>     On 11.03.2014 19:04, Martin Buchholz wrote:
>>>     Thanks for working on my brittle racy tests.
>>>
>>>     Adding a latch as you did is a good improvement, although there's
>>>     a good chance the real problem lies elsewhere.
>>>
>>>     My standard name for such latches is "threadsStarted", which I
>>>     think is a bit better than "startedSignal".  Please rename.
>>>
>>     No problem, renamed.
>>
>>
>>>     My simple calls to Thread.join are too optimistic.
>>>     More likely to be helpful is code like (pseudocode follows):
>>>
>>>     thread.join(10, SECONDS);
>>>     if (thread.isAlive()) {
>>>       dumpAllStacks();
>>>       fail();
>>>     }
>>>
>>     Yes, it was a good idea to do that!
>>     After implementing your suggestion, I could finally reproduce the
>>     failure.
>>     The OpenLoop child thread was spinning in the do-while loop,
>>     waiting for 'count of fds in use' to become 3.
>>     The simple solution is to add a check weather the current thread
>>     is interrupted to this and other loops.
>>
>>     Would you please take a look at the updated webrev?
>>     http://cr.openjdk.java.net/~igerasim/8034262/1/webrev/
>>     <http://cr.openjdk.java.net/%7Eigerasim/8034262/1/webrev/>
>>
>>     Sincerely yours,
>>     Ivan
>>
>>
>>>     That's  more work to implement - optional.
>>>
>>>
>>>     On Tue, Mar 11, 2014 at 5:24 AM, Ivan Gerasimov
>>>     <ivan.gerasimov at oracle.com <mailto:ivan.gerasimov at oracle.com>>
>>> wrote:
>>>
>>>         Hello everybody!
>>>
>>>         The test java/lang/ProcessBuilder/CloseRace.java was reported
>>>         to intermittently fail.
>>>         The test timed out, which should mean that at least one of
>>>         the child threads was never interrupted.
>>>
>>>         I couldn't reproduce the failure, but I suspect it might
>>>         happen due to call to interrupt() before the child thread
>>>         became alive (I'm not really sure if it's possible to be
>>>         non-alive after call to start()).
>>>         The fix is to explicitly synchronize children with the parent.
>>>
>>>         Would you please help review the fix?
>>>
>>>         BUGURL: https://bugs.openjdk.java.net/browse/JDK-8034262
>>>         WEBREV:
>>>         http://cr.openjdk.java.net/~igerasim/8034262/0/webrev/
>>>         <http://cr.openjdk.java.net/%7Eigerasim/8034262/0/webrev/>
>>>
>>>         Sincerely yours,
>>>         Ivan
>>>
>>>
>>
>>
>