On 15/10/2014 11:55 PM, Jaroslav Bachorik wrote:
On 10/15/2014 10:11 AM, David Holmes wrote:
On 15/10/2014 5:50 PM, Jaroslav Bachorik wrote:
On 10/15/2014 02:10 AM, David Holmes wrote:
On 14/10/2014 8:46 PM, Jaroslav Bachorik wrote:
Please, review the following test change
Issue : https://bugs.openjdk.java.net/browse/JDK-8056143
Webrev: http://cr.openjdk.java.net/~jbachorik/8056143/webrev.00
The method jdk.testlibrary.ProcessTools.getOutput(process) waits for
the
given process to finish (process.waitFor()) before grabbing its
outputs.
However, the code does not handle the process.waitFor() being
interrupted correctly - it just goes ahead and tries to obtain the
exit
code which will fail and leave the tested process running.
The correct way is to forcibly destroy the process when
process.waitFor() is interrupted or throws ExecutionException to make
sure the process has actually exited before checking its exit code.
Why is this correct? What gives the thread calling getOutput the right
to terminate the target process just because that thread was
interrupted
while waiting? If the interrupting thread intended the interrupt to
mean
"forcibly terminate the process and interrupt all threads waiting on
it"
then that thread should be doing the termination _not_ the one that was
interrupted!
Process.waitFor() gets interrupted by a thread unknown to the actual
test case - probably the JTreg timeout thread. The interrupting thread
doesn't know that it is supposed to destroy a process. Once JTreg can
take care of cleaning up process tree upon exit this code wouldn't be
needed.
I was contemplating adding the check for "null" returned from
ProcessTools.getOutput() and destroying the process inside the caller
code - but this would have the same results as doing it in
ProcessTools.getOutput() with the drawback of duplicating the same check
everywhere ProcessTools.getOutput() would be used.
A silent postcondition of ProcessTools.getOuptut() is that the target
process has finished - and it holds for all the code paths except the
InterruptedException handler.
That doesn't mean it is up to getOutput to forcibly terminate the
process. Multi-process cancellation is tricky, and yes eventually jtreg
will handle it. But this seems the wrong place to handle it now. Part of
the flaw here is that getOutput should itself throw InterruptedException
so that the caller is forced to deal with this - instead it just
re-asserts the interrupt state. The caller has to be aware that the
thread can be interrupted and do something appropriate - which may mean
punting to its caller. This is akin to a thread catching
InterruptedException and calling System.exit - it simply is not its job
to make that kind of decision at that level!
There is no other decision to make. Not as it is written today. You can
call ProcessTools.getOutput() and check whether the result is null and
then end the test process. There is no other sensible action. The
Process.waitFor() was interrupted you have no data to perform the checks
against so the test will fail and as such it should stop any external
processes it has started.
Yes, I can go through all the tests using ProcessTools.getOutput() and
add `if (output == null) process.destroyForcible();` - would this make
it a better solution than putting this logic inside
ProcessTools.getOutput()?
It would be the correct solution. Hacking it into getOutput() is just a
convenience. Problem is that none of these tests have given enough
thought to the cancellation issue and general process management.
Sorry.
David
-JB-
David
-JB-
David
Thanks,
-JB-
