RFR(s) #2: 6344935: (spec) clarify specifications for Object.wait overloads

[Peter Levart]

Hi Martin,

On 08/21/2017 05:05 AM, Martin Buchholz wrote:
> On Sat, Aug 19, 2017 at 1:53 PM, Martin Buchholz <martinrb at google.com>
> wrote:
>
>> Now I see that the code snippet in TimeUnit.timedWait is also in need of fixing. Hmmmm ....
>>
>>   public synchronized Object poll(long timeout, TimeUnit unit)
>>       throws InterruptedException {
>>     while (empty) {
>>       unit.timedWait(this, timeout);
>>       ...
>>     }
>>   }
>>
>> It's surprisingly hard to write a high quality wait loop, and we've been
> doing our users a disservice by punting on providing good models in the
> javadoc.  You probably want to work entirely in nanoseconds, to avoid
> rounding errors, and because you'll end up calling nanoTime anyways.  You
> don't want to call nanoTime unless you are sure to wait.  OTOH you want to
> throw NPE on null TimeUnit even if there's no need to wait.  You must
> beware of overflow when timeout is Long.MIN_VALUE.
>
> Below is my attempt to fix the sample loop in timedWait.  Calling
> Object.wait directly is obviously even harder.  How about just not
> providing a sample loop that calls Object.wait directly (effectively,
> gently deprecate timed Object wait in favor of the loop below)  (I'm also
> pretending that the platform has better than millisecond granularity).
>
>   public E poll(long timeout, TimeUnit unit)
>       throws InterruptedException {
>     long nanosTimeout = unit.toNanos(timeout);
>     synchronized (lock) {
>       for (long deadline = 0L; isEmpty(); ) {
>         long remaining = (deadline == 0L) ? nanosTimeout
>           : deadline - System.nanoTime();
>         if (remaining <= 0)
>           return null;
>         if (deadline == 0L) { // first wait
>           deadline = System.nanoTime() + nanosTimeout;
>           if (deadline == 0L) // unlikely corner case
>             deadline = 1L;
>         }
>         NANOSECONDS.timedWait(lock, remaining);
>       }
>       return dequeueElement();
>     }
>   }

You could split the state of "first wait" vs. "deadline" into separate 
locals. It would be easier to understand and no corner case to watch 
for. For example:

     public E poll(long timeout, TimeUnit unit)
         throws InterruptedException {
         long nanosTimeout = unit.toNanos(timeout);
         long deadline = 0;
         long remaining = nanosTimeout;
         synchronized (lock) {
             for (boolean first = true; isEmpty();) {
                 if (remaining <= 0)
                     return null;
                 if (first) { // before first wait
                     deadline = System.nanoTime() + nanosTimeout;
                     first = false;
                 }
                 NANOSECONDS.timedWait(lock, remaining);
                 remaining = deadline - System.nanoTime();
             }
             return dequeueElement();
         }
     }


Regards, Peter