Request to review JDK-8028094
Balchandra Vaidya
balchandra.vaidya at oracle.com
Wed Nov 27 12:38:41 UTC 2013
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.vaidya at oracle.com <mailto: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
>>> <mailto: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/
>>> <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
>>>
>>>
>>>
>>
>
>
More information about the core-libs-dev
mailing list