Iterators.cycle() does not support concurrently modified collections

[brandontoner]

The Iterators.cycle() does not correctly function when restarting the iteration if Iterable has been modified between calls to hasNext() and next().

List<String> list = new CopyOnWriteArrayList<>(Collections.singletonList("test"));
Iterator<String> iterator = Iterators.cycle(list);
iterator.hasNext(); // returns true, so next must not throw
list.remove(0); // remove value from the collection, possibly done by another thread
iterator.next(); // throws NoSuchElementException

The same pattern, without Iterators.cycle() works correctly

List<String> list = new CopyOnWriteArrayList<>(Collections.singletonList("test"));
Iterator<String> iterator = list.iterator();
iterator.hasNext();
list.remove(0);
iterator.next(); // does not throw

This is due to the iterator method returned from Iterators.cycle() using a different instance of the iterator in the hasNext() and next() methods if the current iterator has no elements left. Since the contract between hasNext() and next() only applies to a single instance, next() can throw incorrectly.

A potentially less standard use case that would also be incompatible is when the Iterable.iterator().hasNext() is not idempotent.

List<String> list = new ArrayList<>(Collections.singletonList("test"));
Iterator<String> iterator = Iterators.cycle(() -> Iterators.filter(Iterators.consumingIterator(list.iterator()),
                                                                   ignored -> true));
iterator.hasNext(); // returns true
iterator.next(); // throws

The iterator returned by Iterators.filter()'s hasNext() method calls next() on the downstream iterator. Since the downstream iterator removes the only element on next(), the next invocation will return an empty iterator.

[cgdecker]

Iterators.cycle() doesn't make any guarantees that it'll work with concurrently modified collections, but it also doesn't warn against it. It does seem like we could perhaps improve the behavior here.