Hi Martin,
I have incorporated your contribution into following
webrev. Please review the changes.
http://cr.openjdk.java.net/~bvaidya/8/8028094/webrev.01/
I have tested it on a linux machine, and the test is passing.
The sleep processes are killed when the pkill command exists
on the machine. The test is passing without killing sleep when the
pkill command is not present.
Thanks
Balchandra
On 11/26/13 06:15 PM, Martin Buchholz wrote:
I dredged up some more memories about this part of ProcessBuilder and
this part of Basic.java.
As the comment indicates, there are remaining issues (might even call
it a "bug") where the grandchild can keep a file descriptor open and
thus cause a hang in the java process. Which is very hard to fix; I
advise you not to try, unless perhaps your name is Alan Bateman.
But here's an improvement to Basic.java which should kill off the
sleep if pkill is available, not fail if pkill is not available, and
keep the wakeupJeff-protected code working as intended until some
brave soul tries to tackle the lingering bug in ProcessBuilder.
https://www.youtube.com/watch?v=XP7q2o1Z0w8
--- a/test/java/lang/ProcessBuilder/Basic.java
+++ b/test/java/lang/ProcessBuilder/Basic.java
@@ -2017,7 +2017,6 @@
&& new File("/bin/bash").exists()
&& new File("/bin/sleep").exists()) {
final String[] cmd = { "/bin/bash", "-c",
"(/bin/sleep 6666)" };
- final String[] cmdkill = { "/bin/bash", "-c",
"(/usr/bin/pkill -f \"sleep 6666\")" };
final ProcessBuilder pb = new ProcessBuilder(cmd);
final Process p = pb.start();
final InputStream stdout = p.getInputStream();
@@ -2045,13 +2044,13 @@
stdout.close();
stderr.close();
stdin.close();
- new ProcessBuilder(cmdkill).start();
//----------------------------------------------------------
// There remain unsolved issues with asynchronous close.
// Here's a highly non-portable experiment to
demonstrate:
//----------------------------------------------------------
if (Boolean.getBoolean("wakeupJeff!")) {
System.out.println("wakeupJeff!");
+ long startTime = System.nanoTime();
// Initialize signal handler for INTERRUPT_SIGNAL.
new
FileInputStream("/bin/sleep").getChannel().close();
// Send INTERRUPT_SIGNAL to every thread in this
java.
@@ -2064,8 +2063,18 @@
};
new ProcessBuilder(wakeupJeff).start().waitFor();
// If wakeupJeff worked, reader probably got EBADF.
- reader.join();
+ long timeout = 60L * 1000L;
+ reader.join(timeout);
+ long elapsedTimeMillis =
+ (System.nanoTime() - startTime) / (1024L *
1024L);
+ check(elapsedTimeMillis < timeout);
+ check(!reader.isAlive());
}
+ // Try to clean up the sleep process, but don't fret
about it.
+ try {
+ new ProcessBuilder("/usr/bin/pkill", "-f", "sleep
6666")
+ .start();
+ } catch (IOException noPkillCommandInstalled) { }
}
} catch (Throwable t) { unexpected(t); }
On Thu, Nov 21, 2013 at 4:26 AM, Balchandra Vaidya
<balchandra.vai...@oracle.com <mailto:balchandra.vai...@oracle.com>>
wrote:
Hi Martin,
> + check(elapsedTimeMillis < timeout);
> + *check(!reader.isAlive());*
The line check(!reader.isAlive()) is causing the test failure
when the pkill command is not available. Any idea ? The
test is passing with check(reader.isAlive()).
The modified test is passing when the pkill command is
available.
When the pkill command is not available, the test is/was
failing without try block around
new ProcessBuilder(cmdkill).start().
So, without placing the above line under try block was a
mistake. I will make the correction.
Thanks
Balchandra
On 11/20/13 03:11 PM, Balchandra Vaidya wrote:
Thanks Martin. I will incorporate your suggested improvements, and
will send out another webrev.
Regards
Balchandra
On 19/11/2013 22:53, Martin Buchholz wrote:
I see this is already submitted.
I thought I could do better than using pkill, but no - there is
no convenient communication from the java process to the
grandchild. This is a hairy test!
Nevertheless, I would like you to incorporate the following
improvements:
- invoke pkill directly
- do some extra checking
- join with reader thread
- don't fail if pkill is not available
--- a/test/java/lang/ProcessBuilder/Basic.java
+++ b/test/java/lang/ProcessBuilder/Basic.java
@@ -2016,7 +2016,7 @@
&& new File("/bin/bash").exists()
&& new File("/bin/sleep").exists()) {
final String[] cmd = { "/bin/bash", "-c",
"(/bin/sleep 6666)" };
- final String[] cmdkill = { "/bin/bash", "-c",
"(/usr/bin/pkill -f \"sleep 6666\")" };
+ final String[] cmdkill = { "/usr/bin/pkill",
"-f", "sleep 6666" };
final ProcessBuilder pb = new ProcessBuilder(cmd);
final Process p = pb.start();
final InputStream stdout = p.getInputStream();
@@ -2044,7 +2044,9 @@
stdout.close();
stderr.close();
stdin.close();
- new ProcessBuilder(cmdkill).start();
+ // Try to clean up the sleep process, but don't
fret about it.
+ try { new ProcessBuilder(cmdkill).start(); }
+ catch (IOException noPkillCommandInstalled) { }
//----------------------------------------------------------
// There remain unsolved issues with
asynchronous close.
// Here's a highly non-portable experiment to
demonstrate:
@@ -2063,8 +2065,14 @@
};
new
ProcessBuilder(wakeupJeff).start().waitFor();
// If wakeupJeff worked, reader probably
got EBADF.
- reader.join();
}
+ long startTime = System.nanoTime();
+ long timeout = 60L * 1000L;
+ reader.join(timeout);
+ long elapsedTimeMillis =
+ (System.nanoTime() - startTime) / (1024L *
1024L);
+ check(elapsedTimeMillis < timeout);
+ check(!reader.isAlive());
}
} catch (Throwable t) { unexpected(t); }
On Tue, Nov 19, 2013 at 4:21 AM, Balchandra Vaidya
<balchandra.vai...@oracle.com
<mailto:balchandra.vai...@oracle.com>> wrote:
Hi,
Here is one possible solution for the issue JDK-8028094.
http://cr.openjdk.java.net/~bvaidya/8/8028094/webrev/
<http://cr.openjdk.java.net/%7Ebvaidya/8/8028094/webrev/>
I am not sure pkill is available in all Unix flavor at
/usr/bin directory,
but it is present in Solaris and OEL 6. I have tested on Solaris
and OEL and "sleep 6666" is no longer left over.
Thanks
Balchandra
