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

[Martin Buchholz]

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;

1964                 String os = System.getProperty("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.

1976                         useCount = (Integer)useCountField.get(s);

I think you want to use getInt (not get) on a field of type int.

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();
}
}

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"?

Thanks,

Martin

On Sun, Oct 7, 2012 at 12:48 PM, Rob McKenna <rob.mckenna at oracle.com> wrote:

>  Thanks Martin,
>
> I've uploaded a new webrev to:
>
> http://cr.openjdk.java.net/~robm/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>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/>
>>>
>>> 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
>>
>
>
>