guava icon indicating copy to clipboard operation
guava copied to clipboard

ConsumingQueueIterator is not thread safe

Open bjrke opened this issue 3 years ago • 0 comments

The code of ConsumingQueueIterator is not thread safe because it is not using an atomic operation in computeNext

we have this stacktrace once in our whole cluster

java.util.NoSuchElementException: null
    at java.util.AbstractQueue.remove(AbstractQueue.java:117)
    at com.google.common.collect.ConsumingQueueIterator.computeNext(ConsumingQueueIterator.java:44)
    at com.google.common.collect.AbstractIterator.tryToComputeNext(AbstractIterator.java:146)
    at com.google.common.collect.AbstractIterator.hasNext(AbstractIterator.java:141)
    ...

Instead of

  public T computeNext() {
    // TODO(b/192579700): Use a ternary once it no longer confuses our nullness checker.
    if (queue.isEmpty()) {
      return endOfData();
    }
    return queue.remove();
  }

I suggest sth like this but this changes the semantics since queued nulls mess this up... ofc its questionable if it makes sense at all

  protected T computeNext() {
    final var result = queue.poll();
    return result == null ? endOfData() : result;
  }

for me its also unclear why computeNext is public...

bjrke avatar Aug 09 '22 16:08 bjrke