Skip to content

Conversation

@nickita-khylkouski
Copy link

Summary

  • Override spliterator() in TransformingSequentialList and TransformingRandomAccessList to delegate to the backing list's spliterator via CollectSpliterators.map()
  • This preserves characteristics like IMMUTABLE and CONCURRENT from the backing list
  • Add test verifying IMMUTABLE characteristic preservation with CopyOnWriteArrayList

Motivation

When using Lists.transform() with a CopyOnWriteArrayList, concurrent modifications cause spurious ConcurrentModificationException:

var cow = new CopyOnWriteArrayList<Integer>();
Thread.ofPlatform().daemon().start(() -> {
    while (true) {
        for (int i = 0; i < 100_000; i++) cow.add(i);
        cow.clear();
    }
});
var view = Lists.transform(cow, s -> s);
while (true) {
    view.stream().forEach(integer -> {}); // Throws CME!
}

Root cause: CopyOnWriteArrayList.spliterator() has the IMMUTABLE characteristic, which tells the Stream API it's safe to iterate without CME concerns. However, Lists.transform() returns a list that inherits from AbstractList, whose default spliterator() uses RandomAccessSpliterator - this does NOT preserve the IMMUTABLE characteristic, causing index-based iteration that throws IndexOutOfBoundsException (converted to CME) when the list is modified.

The fix: Override spliterator() to use CollectSpliterators.map(), which preserves IMMUTABLE, CONCURRENT, ORDERED, SIZED, and SUBSIZED characteristics from the backing spliterator. This matches the pattern already used in Collections2.TransformedCollection.

Testing

  • All 6000 ListsTest tests pass
  • New test testTransformSpliteratorPreservesCharacteristics() verifies IMMUTABLE is preserved
  • Manual verification: 0 CME exceptions over 18+ million iterations with fix vs 28,000+ CME exceptions without fix

Compatibility

This change only affects the spliterator behavior, not iteration via Iterator. The spliterator will now correctly report characteristics inherited from the backing list, which is strictly more correct behavior.

Fixes #8165

RELNOTES=Lists.transform() now preserves spliterator characteristics (like IMMUTABLE) from the backing list, preventing spurious ConcurrentModificationException when wrapping concurrent lists.

@lowasser
Copy link
Contributor

I will admit my primary concern is that it is not obvious to me that we can correctly guarantee the semantics of these properties. Are they preserved e.g. by Stream.map?

@nickita-khylkouski
Copy link
Author

Yes, IMMUTABLE and CONCURRENT are preserved by Stream.map(). The JDK's ReferencePipeline.map() only clears SORTED and DISTINCT (via StreamOpFlag.NOT_SORTED | StreamOpFlag.NOT_DISTINCT). There aren't any NOT_IMMUTABLE or NOT_CONCURRENT flags in StreamOpFlag at all, the pipeline just doesn't track these.

This makes sense because both characteristics describe the source, not the elements:

  • IMMUTABLE: "the element source cannot be structurally modified"
  • CONCURRENT: "the element source may be safely concurrently modified"

A mapping function transforms elements but doesn't change whether the underlying source is immutable or concurrent-safe. That's different from SORTED/DISTINCT, which describe element relationships that mapping can break.

Guava's own CollectSpliterators.map() already preserves both of these. Lines 142-148 mask the backing characteristics to SIZED | SUBSIZED | ORDERED | IMMUTABLE | CONCURRENT with the comment "the following are inherently inherited." Collections2.TransformedCollection.spliterator() already relies on this behavior.

@cpovirk
Copy link
Member

cpovirk commented Jan 28, 2026

I may have done the testing wrong, but my testing indicated that both IMMUTABLE and CONCURRENT were cleared by Stream.map. I see the same code as you, but I think there's another level of clearing that happens inside AbstractPipeline, which applies OP_MASK, and presumably that's what's responsible.

I do agree that there's an argument for keeping both; I just don't think we can argue that on the grounds of matching JDK behavior.

@nickita-khylkouski
Copy link
Author

You're right, I just tested it and Stream.map() does drop both. That's AbstractPipeline's OP_MASK stripping them since they're not tracked in StreamOpFlag. My mistake on the JDK comparison.

Though worth noting this PR doesn't change that behavior. CollectSpliterators.map() already preserves IMMUTABLE/CONCURRENT (lines 142-148), and Collections2.TransformedCollection already relies on it. This PR just has Lists.transform use that same existing path.

@nickita-khylkouski
Copy link
Author

Superseded by PR #8182 which includes Lists.transform fixes along with comprehensive spliterator characteristic preservation for issue #8165

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 no SLO

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lists::transform and probably others do not produce spliterators/iterators with correct characteristics

4 participants