RFR: 7152183: TEST_BUG: java/lang/ProcessBuilder/Basic.java failing intermittently [sol]

[Rob McKenna]

Thanks Martin,

I've uploaded a new webrev to:

http://cr.openjdk.java.net/~robm/7152183/webrev.02/ 
<http://cr.openjdk.java.net/%7Erobm/7152183/webrev.02/>

Let me know if this does the job.

     -Rob

On 04/10/12 18:24, Martin Buchholz wrote:
> Hi all,
>
> Yeah, this particular test is rather racy - sorry about that.
> We need to call p.destroy when the other thread is in the middle of a 
> read() system call, and there's no way to know for sure - seeing java 
> "read" in the stacktrace is not enough, since it may not have gotten 
> to the system call yet.
>
> suggestions:
> pull the computation of the inputstream before the latch to narrow the 
> window a bit:
>
> final InputStream s;
> switch (action & 0x1) {
> case 0: s = p.getInputStream(); break;
> case 1: s = p.getErrorStream(); break;
> default: throw
> }
> latch.countdown();
> switch (action & 0x2) {
> case 0: r = s.read(); break;
> case 1: r = s.read(bytes); break;
> }
>
> Examining the stack trace to look for "read" is clever but does not 
> actually eliminate the race.
>
> Looking in UNIXProcess.java.solaris I see the use 
> of DeferredCloseInputStream.  We can eliminate the race on solaris 
> (i.e. if the inputstream.getclass isDeferredCloseInputStream)  by 
> looping until the useCount field of the DeferredCloseInputStream is > 
> 0, using ugly but effective reflective code.  This should allow us to 
> avoid the horrible sleep for 200ms.
>
> You should use yield instead of sleep between loop iterations while 
> waiting for the useCount to be bumped.
>
> On other platforms this is not an issue, I think.
>
> Martin
>
> On Thu, Oct 4, 2012 at 12:39 AM, Alan Bateman <Alan.Bateman at oracle.com 
> <mailto:Alan.Bateman at oracle.com>> wrote:
>
>     On 03/10/2012 22:44, Rob McKenna wrote:
>
>         Hi folks,
>
>         The only way I can see this test failing in this manner[*] is
>         if we destroy the process before we begin the read. That being
>         the case I've jacked up the sleep (giving the reader thread a
>         little more time to get cracking) and added a check to see if
>         the threads stack has entered a read call.
>
>         http://cr.openjdk.java.net/~robm/7152183/webrev.01/
>         <http://cr.openjdk.java.net/%7Erobm/7152183/webrev.01/>
>         <http://cr.openjdk.java.net/%7Erobm/7152183/webrev.01/>
>
>         Feedback greatly appreciated.
>
>             -Rob
>
>
>         [*] le trace:
>
>     So stack traces are masculine, I didn't know that.
>
>     I think your analysis is right, it's just that the sleep(10) is
>     not sufficient to ensure that the thread gets to the read method.
>     Increasing the sleep is probably sufficient. The hack to look at
>     the stack trace makes it more robust for really extreme cases, at
>     the cost of potential further maintenance in the event that the
>     implementation changes. In any case it's good to resolve this
>     intermittent test failure.
>
>     -Alan
>
>