Request to review JDK-8028094
Martin Buchholz
martinrb at google.com
Tue Nov 26 18:15:19 UTC 2013
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.vaidya at 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.vaidya at oracle.com> wrote:
>
>>
>> Hi,
>>
>> Here is one possible solution for the issue JDK-8028094.
>> http://cr.openjdk.java.net/~bvaidya/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
>>
>>
>>
>
>
>
More information about the core-libs-dev
mailing list