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.mcke...@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.bate...@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
>>
>
>
>
