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

[Rob McKenna]

Thanks Martin,

Updated webrev at:

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

and a few comments inline:

On 08/10/12 18:52, Martin Buchholz wrote:
> Thanks for the changes - this approach looks good.  But:
>
> 1954                                 case 0: r = s.read(bytes); break;
> 1955                                 case 2: r = s.read(bytes); break;
>
> The two cases are the same code; you want
>
> case 0: r = s.read(); break;
Eesh, typo, sorry.
>
> 1964                 String os = System.getProperty("os.name 
> <http://os.name>");
> 1965                 if (os.equalsIgnoreCase("Solaris") ||
> 1966                     os.equalsIgnoreCase("SunOS"))
>
> I wouldn't bother with testing for Solaris explicitly, and just rely 
> on reflective code testing the implementation of s.
> Loop for useCount to go non-negative If and only if we find it 
> reflectively.
>
> 1970                     if (s.toString().startsWith(
> 1971 "java.lang.UNIXProcess$DeferredCloseInputStream"))
>
> It's bad style to depend on the output of toString() - better is 
> something like
>
> Class<?> c = s.getClass();
> if (c.getName().equals(...)) {
>
> Oh, now I see the difficulty - the DeferredCloseInputStream may or may 
> not be wrapped in a BufferedInputStream.
> Which makes the reflective code much more annoying.
Yes, that being the case I'm going to leave in the OS check, is that ok? 
Also yes, c.getName() makes far more sense, thanks.
>
> 1976                         useCount = (Integer)useCountField.get(s);
>
> I think you want to use getInt (not get) on a field of type int.
Yes I do.
>
> 1987                     while (useCount.intValue() <= 0) {
> 1988                         useCount = (Integer)useCountField.get(s);
> 1989 Thread.currentThread().yield();
> 1990                     }
>
> I was imagining a loop that looks more like this:
>
> if (useCountField != null) {
> while (useCountField.getInt(s) <= 0) {
>   Thread.currentThread().yield();
> }
> }
Thats much nicer. I was thinking I'd leave the null check out as if 
useCountField is null at this point, the test should probably fail. Let 
me know if thats ok.
>
> I'm surprised you're not seeing IAE when calling useCountField.get(s) 
> when wrapped in a BIS.  Shouldn't that be a call 
> to useCountField.get(deferred) instead?  Can we fix this in the 
> wrapped case by assigning "s = deferred"?
>
Sorry, that was a mistake. Rectified now. I hadn't actually exercised 
that code due to my while loop implementation and the fact that the 
problem wasn't reproducing. Thanks for spotting that.

     -Rob
> Thanks,
>
> Martin
>
> On Sun, Oct 7, 2012 at 12:48 PM, Rob McKenna <rob.mckenna at oracle.com 
> <mailto:rob.mckenna at oracle.com>> wrote:
>
>     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
>>
>>
>
>