On 10/26/2014 11:53 PM, David Holmes wrote:
On 25/10/2014 3:06 AM, Jaroslav Bachorik wrote:
Please, review this change to the test library.
Issue : https://bugs.openjdk.java.net/browse/JDK-8062070
Webrev: http://cr.openjdk.java.net/~jbachorik/8062070/webrev.00
The test started failing after the process.destroyForcibly() was added
to ProcessTools.executeProcess() to make sure there were no orphaned
processes once the test was finished. The destruction call was placed to
the finally block and was called for running or terminated processes
indiscriminately.
It turns out that Process.get*Stream() methods will get confused when
calling Process.destroy/Forcibly/() on an already exited process - the
streams will get closed and any attempt to read or write to them will
end with a SocketException.
The failure is timing related - when the stream manages to buffer data
before destroying the process from another thread the test passes.
Otherwise it just fails.
The solution is to forcibly close the external process (started by
ProcessTools.executeProcess()) only in cases when eg. Process.waitFor()
throws an exception - moving it from the finally block to the catch
block.
I can't help but think we are simply shifting the failure window around.
But the proof of this is in the testing.
I don't know whether we can do any better while keeping the
post-condition of having the process exited before leaving this method.
But with this change we would hit this problem in cases when the test
would be already failing anyway.
I ran JPRT on all the available platforms and the test didn't fail once.
Thanks for the reviews.
-JB-
Reviewed.
David
Thanks,
-JB-
