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.mcke...@oracle.com
<mailto:rob.mcke...@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.bate...@oracle.com <mailto: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/>
<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
