Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConcurrentModificationException iterating over partitions #1252

Closed
johnny-costanzo opened this issue Mar 31, 2020 · 3 comments
Closed

ConcurrentModificationException iterating over partitions #1252

johnny-costanzo opened this issue Mar 31, 2020 · 3 comments
Labels
type: bug A general bug
Milestone

Comments

@johnny-costanzo
Copy link

Bug Report

I'm calling the readonly method on an async commands instance to select some nodes and hit a ConcurrentModificationException. It looked like a topology refresh happened at the same time and modified the partitions. Looking into the code, Partitions.getPartitions offers up access to its internal list of partitions (which is otherwise synchronized on in all internal methods), but client code that calls getPartitions is not protected. The StaticNodeSelection constructor calls this method and iterates over the list can potentially result in this issue when the list changes. Consider using a CopyOnWriteArrayList or some other mechanism so that clients of the getPartitions call can safely iterate over the list.

Current Behavior

Stack trace
java.util.ConcurrentModificationException
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1660)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
        at io.lettuce.core.cluster.StaticNodeSelection.<init>(StaticNodeSelection.java:51)
        at io.lettuce.core.cluster.RedisAdvancedClusterAsyncCommandsImpl.nodes(RedisAdvancedClusterAsyncCommandsImpl.java:542)
        at io.lettuce.core.cluster.RedisAdvancedClusterAsyncCommandsImpl.readonly(RedisAdvancedClusterAsyncCommandsImpl.java:522)

Environment

  • Lettuce version(s): 5.2.0.RELEASE
@johnny-costanzo johnny-costanzo added the type: bug A general bug label Mar 31, 2020
@johnny-costanzo
Copy link
Author

or possibly this use case should have been querying from Partitions internal nodeReadView instead of calling getPartitions() since it doesn't look like that collection mutates

@mp911de mp911de added this to the 5.3.0 milestone Apr 1, 2020
mp911de added a commit that referenced this issue Apr 1, 2020
The node-selection API now uses the read-only view instead of the mutable partitions view when consuming the topology view.
mp911de added a commit that referenced this issue Apr 1, 2020
The node-selection API now uses the read-only view instead of the mutable partitions view when consuming the topology view.
mp911de added a commit that referenced this issue Apr 1, 2020
The node-selection API now uses the read-only view instead of the mutable partitions view when consuming the topology view.
@mp911de
Copy link
Collaborator

mp911de commented Apr 1, 2020

Thanks for the report. The described approach is exactly what we need to do here. StaticNodeSelection (and a couple other places) use the mutable collection while they should consume the read-only one that is not mutated once returned from the Partitions object.

@mp911de
Copy link
Collaborator

mp911de commented Apr 1, 2020

That's fixed now.

@mp911de mp911de closed this as completed Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants