RFR-8148748: ArrayList.subList().spliterator() is not late-binding

[Paul Sandoz]

> On 4 Mar 2016, at 17:42, Tagir F. Valeev <amaembo at gmail.com> wrote:
> 
> Hello!
> 
> AIOOBE is possible for ArrayList itself as well. E.g.:
> 
>    ArrayList<Integer> test = new ArrayList<>(Arrays.asList(1,2,3,4));
>    Spliterator<Integer> spltr = test.spliterator();
>    spltr.tryAdvance(System.out::println);
>    test.clear();
>    test.trimToSize();
>    spltr.tryAdvance(System.out::println);
> 
> Result (both in Java-8 and Java-9):
> 
> 1
> Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 1
>    at java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1398)
> 
> So this is not subList-specific problem.
> 

Yes, that was the first aspect i mentioned. Generally for bulk traversal we opted to throw the CME at the end, which i think is a reasonable compromise, especially since mixed traversal is an edge case. I was not suggesting we revisit that.

I was concerned about the sublist case, then I recalled a co-mod check is performed *before* construction:

1282         public Spliterator<E> spliterator() {
1283             checkForComodification();
1284
1285             // ArrayListSpliterator is not used because late-binding logic
1286             // is different here
1287             return new Spliterator<>() {

I forgot about that! sorry for the noise. We are good. I will push on Monday.

Thanks,
Paul.


> Seems that forEachRemaining is not affected by this, due to additional
> length check and making the local copy of elementData array. At least
> I don't see the way to make forEachRemaining (either of subList
> spliterator or main ArrayList spliterator) throwing AIOOBE. Probably
> I'm missing something. However it can traverse some unexpected nulls
> before throwing CME if it was shrinked:
> 
>    ArrayList<Integer> test = new ArrayList<>(Arrays.asList(1,2,3,4));
>    Spliterator<Integer> spltr = test.spliterator();
>    spltr.tryAdvance(System.out::println);
>    test.clear();
>    spltr.forEachRemaining(System.out::println);
> 
> Output:
> 
> 1
> null
> null
> null
> Exception in thread "main" java.util.ConcurrentModificationException
>    at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1428)
> 
> This may unexpectedly throw NPE if forEachRemaining Consumer
> unconditionally dereference them as program logic suggests that null
> is impossible here. So user will see NPE instead of CME.
> 
> Probably it could be logged as separate issue. My patch does not cause
> any regression here (at least, I hope so). Probably some other
> collections should be checked for similar corner cases as well.
> 
> With best regards,
> Tagir Valeev.
> 
>>> On 4 Mar 2016, at 15:38, Tagir F. Valeev <amaembo at gmail.com> wrote:
>>> 
>>> Hello!
>>> 
>>> Thank you for the review!
>>> 
> 
> PS> Thanks.
> 
> PS> I just realised there are some subtleties where if the top-level
> PS> list is reduced in size the spliterator of a sublist may on
> PS> traversal throw an ArrayIndexOutOfBoundsException rather than ConcurrentModificationException.
> 
> PS> This can already occur for a partially traversed top-level list
> PS> spliterator, so i am wondering how much we should care. Arguably
> PS> in the sublist case there is another level of indirection, where
> PS> errors creep in before traversal, so it suggests we should
> PS> additionally check the mod count at the start of the traversal
> PS> methods, and it’s probably ok on a best-effort basis to do this
> PS> for the sublist spliterator and not it’s splits.
> 
> PS> Separately, i am not sure why the SubList.iterator has to check
> PS> that the offset + 1 is within bounds, since expectedModCount =
> PS> ArrayList.this.modCount should be false.
> 
> PS> Paul.
> 
>>> Here's updated webrev:
>>> http://cr.openjdk.java.net/~tvaleev/webrev/8148748/r3/
>>> 
>>> PS> Looks good. I especially like:
>>> 
>>> PS>  125             addCollection(l.andThen(list -> list.subList(0, list.size())));
>>> 
>>> PS> Can you also update SpliteratorTraversingAndSplittingTest?
>>> 
>>> PS> void addList(Function<Collection<T>, ? extends List<T>> l) {
>>> PS>     // @@@ If collection is instance of List then add sub-list tests
>>> PS>     addCollection(l);
>>> PS> }
>>> 
>>> Done.
>>> 
>>>>> Now it's separate anonymous class as you suggested.
>>>>> ArrayListSpliterator is untouched. Note that trySplit() can return
>>>>> original ArrayListSpliterator as after the binding their behavior is
>>>>> compatible.
>>>>> 
>>> 
>>> PS> Very nice, might be worth an extra comment noting that. Up to you.
>>> 
>>> Short comments added.
>>> 
>>> With best regards,
>>> Tagir Valeev.
>>> 
>