Request to review JDK-8028094
Martin Buchholz
martinrb at google.com
Wed Nov 27 15:03:54 UTC 2013
Thanks. Looks good.
Sorry for having burdened posterity with this test snippet. IIRC I
originally tried to write an ordinary test, but the test succeeded in
demonstrating there was a (difficult) bug left in the code. Hence the
current state of the test code.
I'll add this to my (fix-when-I-retire) list.
On Wed, Nov 27, 2013 at 4:38 AM, Balchandra Vaidya <
balchandra.vaidya at oracle.com> wrote:
>
> 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> 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