Request to review JDK-8028094

[Balchandra Vaidya]

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
>>
>>
>>
>