review request: 4244896: (process) Provide System.getPid(), System.killProcess(String pid)

[David Holmes]

Jeff,

On 1/06/2012 10:19 AM, Jeff Hain wrote:
> Hi.
>> 185     public boolean waitFor(long timeout, TimeUnit unit)
>> 186         throws InterruptedException {
>> 187         long now = System.nanoTime();
>> 188
>> 189         long end = now +
>> 190             (timeout<= 0 ? 0 : TimeUnit.NANOSECONDS.convert(timeout, unit));
>> 191
>> 192         if (end<= 0) // overflow
>> 193             end = Long.MAX_VALUE;
>> 194
>> 195         long rem = end - now;
>> 196         do {
>> 197             try {
>> 198                 exitValue();
>> 199                 return true;
>> 200             } catch(IllegalThreadStateException ex) {
>> 201                 if(rem>  0)
>> 202                     Thread.sleep(
>> 203                         Math.min(TimeUnit.NANOSECONDS.toMillis(rem) + 1, 100));
>> 204             }
>> 205             rem = end - System.nanoTime();
>> 206         } while(rem>  0);
>> 207         return false;
>> 208     }
>
>
> If System.nanoTime() is close to wrapping, this code would consider overflow even for a not-so-large timeout,

If System.nanoTime is close to wrapping then we have all sorts of 
problems to worry about. But you are right. The way to handle this with 
no overflow issues is to track the elapsed time (System.nanoTime() - 
start) which will always give a positive result when less than 2^63 
nanoseconds elapse. That then gets subtracted from the requested timeout 
to give the rem value.

> and the wait could stop earlier than expected.
> (Also "now" should rather be called "startTime" or so (since at some point it's no longer current time).)
>
> One could do as in ScheduledThreadPoolExecutor, and use an offset on System.nanoTime().

Aside: I don't see that in latest version of STPE.

Cheers,
David
-----




> Or, just remove lines 192-194 : it's not really a problem if "end" wraps, since it should unwrap
> when doing "end - System.nanoTime()" (supposing we don't spend centuries in the method).
>
> Or, only work with delta (also supposing we don't wait for centuries), never explicitly considering
> an "end" value, like this:
>
> public boolean waitFor(long timeout, TimeUnit unit)
>      throws InterruptedException {
>      final long startNS = System.nanoTime();
>
>      final long timeoutNS = (timeout<= 0 ? 0 : unit.toNanos(timeout));
>
>      long remNS = timeoutNS;
>      do {
>          try {
>              exitValue();
>              return true;
>          } catch(IllegalThreadStateException ex) {
>              if(remNS>  0)
>                  Thread.sleep(
>                      Math.min(TimeUnit.NANOSECONDS.toMillis(remNS) + 1, 100));
>          }
>          remNS = timeoutNS - (System.nanoTime() - startNS);
>      } while(remNS>  0);
>      return false;
> }
>
> -Jeff