Neat! +1
-Brent
On 1/14/19 12:05 PM, Roger Riggs wrote:
Hi,
Thanks for the reviews.
As suggested, cleaned up a bit more dead wood.
http://cr.openjdk.java.net/~rriggs/webrev-destroytest-8080569-4/
Thanks, Roger
On 01/14/2019 01:56 PM, Brent Christian wrote:
Hi, Roger
On Windows, the test did not check liveness, but will check it now;
seems desirable.
I think the changes look fine as they are. Additional refactoring
possibilities for your consideration, to take or leave:
* ProcessTest::isAlive() is not used
* killProc() no longer needs a boolean argument
* the killProc() code could be moved into runTest()
-Brent
On 1/14/19 8:56 AM, Roger Riggs wrote:
Please review removing a test for Process.destroy(). [1]
It fails intermittently and is based on an incorrect assumption.
The child is a bash script that uses trap to ignore SIGTERM. The
child is started and then sent SIGTERM.
The child should not terminate. However, there is a race in which in
some cases the child does terminate
with SIGPIPE (not SIGTERM) as a result of destroy() closing the streams.
The Process implementation on Unix closes the streams after sending
the SIGTERM signal
and has since (forever...). But this behavior is not documented.
This test of destroy() is invalid and should be removed. Since both
Mac OS and Windows
already skip the testing of destroy() the test is simplified to
remove it from all cases.
A separate issue[2] has been created to consider documenting the
Process implementations' closing of the streams.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-destroytest-8080569-2/
Thanks, Roger
[1] https://bugs.openjdk.java.net/browse/JDK-8080569
[2] https://bugs.openjdk.java.net/browse/JDK-8216990
