Hi Martin,
On 08/21/2017 05:05 AM, Martin Buchholz wrote:
On Sat, Aug 19, 2017 at 1:53 PM, Martin Buchholz <marti...@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
