Re: Request to review JDK-8028094

[Balchandra Vaidya]

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