[jira] [Comment Edited] (GEODE-8195) ConcurrentModificationException from LocatorMembershipListenerImpl$DistributeLocatorsRunnable.run

["Bruce J Schuchardt (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-8195?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17144989#comment-17144989 ] 

Bruce J Schuchardt edited comment on GEODE-8195 at 6/25/20, 2:57 PM:
---------------------------------------------------------------------

We do need to remove the successfully transmitted LocatorJoinMessage from the collection or it will be sent again and again and again.  That's the point of the removal - it's been transmitted and doesn't need to be sent again.

A {color:blue}{{for}}{color} loop in the form {color:blue}{{for (JoinMessage j: joinMessages)}}{color} implicitly uses an {color:blue}{{Iterator}}{color} to traverse the collection.  The only way to safely remove a value from the collection is to use {color:blue}{{Iterator.remove()}}{color} method.  From the javadoc for this method:

_The behavior of an iterator is unspecified if the underlying collection is modified while the iteration is in progress in any way other than by calling this method._


was (Author: bschuchardt):
We do need to remove the successfully transmitted LocatorJoinMessage from the collection or it will be sent again and again and again.  That's the point of the removal - it's been transmitted and doesn't need to be sent again.

A {{for}} loop in the form {{for (JoinMessage j: joinMessages}} implicitly uses an {{Iterator}} to traverse the collection.  The only way to safely remove a value from the collection is to use {{Iterator.remove()}} method.  From the javadoc for this method:

_The behavior of an iterator is unspecified if the underlying collection is modified while the iteration is in progress in any way other than by calling this method._

> ConcurrentModificationException from LocatorMembershipListenerImpl$DistributeLocatorsRunnable.run
> -------------------------------------------------------------------------------------------------
>
>                 Key: GEODE-8195
>                 URL: https://issues.apache.org/jira/browse/GEODE-8195
>             Project: Geode
>          Issue Type: Bug
>          Components: membership
>            Reporter: Bill Burcham
>            Assignee: Bruce J Schuchardt
>            Priority: Major
>
> this WAN code in {{LocatorMembershipListenerImpl$DistributeLocatorsRunnable.run}}:
> {code}
>             Set<LocatorJoinMessage> joinMessages = entry.getValue();
>             for (LocatorJoinMessage locatorJoinMessage : joinMessages) {
>               if (retryMessage(targetLocator, locatorJoinMessage, attempt)) {
>                 joinMessages.remove(locatorJoinMessage);
>               } else {
> {code}
> modifies the {{joinMessages}} set as it is iterating over the set, resulting in a {{ConcurrentModificationException}}.
> This bug will cause (inter-site) notification of locators (of the presence of a new locator) to fail early if retry is necessary. If we have to retry notifying any locator, and we succeed, we’ll throw the {{ConcurrentModificationException}} and stop trying to notify any of the other locators. See the _Discovery For Multi-Site Systems_ section of the [Overview of Multi-Site Caching|https://geode.apache.org/docs/guide/14/topologies_and_comm/topology_concepts/multisite_overview.html] documentation for an overview of the locator's role in WAN.
> Here is a scratch file that illustrates the issue, throwing {{ConcurrentModificationException}}:
> {code}
> import java.util.HashSet;
> import java.util.Set;
> class Scratch {
>   public static void main(String[] args) {
>     final Set<String> joinMessages = new HashSet<>();
>     joinMessages.add("one");
>     joinMessages.add("two");
>     for( final String entry:joinMessages ) {
>       if (entry.equals("one"))
>         joinMessages.remove(entry);
>     }
>   }
> }
> {code}
> From looking at the Geode code, {{joinMessages}} is not used outside the loop so there is no need to modify it at all—I think we can simply remove this line:
> {code}
>                 joinMessages.remove(locatorJoinMessage);
> {code}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)