Using a boolean flag is reasonable, but there are micro-defects here as
well:
- There's a dead initial store to deadline just to appease the definite
assignment algorithm.
- the scope of deadline and remaining leaks into the post-wait-loop code,
and it would be ugly to introduce a new scope explicitly
- if you wait, you will call System.nanoTime() one more time than
necessary. A high quality wait loop only ever calls nanoTime just before
waiting.
On Mon, Aug 21, 2017 at 12:34 AM, Peter Levart <peter.lev...@gmail.com>
wrote:
> 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
>
>
