review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid) (original) (raw)
Rob McKenna rob.mckenna at oracle.com
Tue Apr 17 14:56:30 UTC 2012
- Previous message: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
- Next message: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
New webrev at:
http://cr.openjdk.java.net/~robm/4244896/webrev.01/
Differences:
implemented Process.waitFor(long timeout). I figured I'd throw it in and let you decide if you want to keep it / replace isAlive.
implemented defaults in Process.java
destroyForcibly now returns the Process object
Updated documentation
Added @Override to all new overrides.
Fixed long line in UNIXProcess_md.c consistent with other functions in that file.
Replaced WaitForSingleObject with GetExitCodeProcess: given the concerns raised it seems to make more sense to leave users who return STILL_ACTIVE as an error code on their own.
Test updated to allow for proper execution of shell script.
-Rob
On 12/04/12 10:05, Alan Bateman wrote:
On 12/04/2012 00:48, Rob McKenna wrote:
Hi folks,
I'm hoping for some feedback on the above. Webrev: http://cr.openjdk.java.net/~robm/4244896/webrev.00/ Thanks for taking this one on, a destroyForcibly is really useful to have (destroy was always misleading given that that is was actually a SIGTERM). The isAlive is also useful, another choice would be a waitFor that takes a timeout. As this patch adds two abstract methods it means there will be a source compatibility issue. Process has been in the platform since JDK1.0 so it likely there are other implementation, perhaps mocked. For destroyForcibly then a reasonable default could invoke the existing destroy but this would need to be specified. A possible default for isAlive is to invoke exitValue and mask the IAE when it hasn't terminated. On the javadoc then destroyForcibly needs to specify the return value, I think I missed this in the original patch that I gave you off-list. Having it return the Process is very useful as it allows for method invocation chaining, exitCode = p.destroyForcibly().waitFor(). I also think destroyForcibly needs to set expectation that the process may not terminate immediately (isAlive might still return true for a brief period for example). The duplicate code in UNIXProcess.java.* is a reminder that we could combine the common code into something like a private package AbstractUNIXPorcess that would be extended by UNIXProcess, not for this patch of course. Looks like there is inconsistent use of @Override, you've added it to the isAlive implementation but not the others. Looks like UNIXProcessmd.c is straying beyond 80c so you might want to move the force parameter to the next line. Overall I think the implementation looks okay but one thing to think about is errors, WaitForSingleObject can fail with other errors, the return from kill is not checked. I don't have time to study the test is detail just now but I suspect invoking "./ProcessKillTest.sh" will need to change as the script will be in test.src and may not have execute permission. Also I see tests for specific exit codes which may be problematic (and will need to be updated for MacOSX). For isAlive then it may be better to test it by adding to existing tests. -Alan.
- Previous message: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
- Next message: review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]