Fix race condition in RemoteClusterConnection node supplier #25432
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes a race condition in the node supplier used by the RemoteClusterConnection. The node supplier stores an iterator over a set backed by a ConcurrentHashMap, but the get operation of the supplier uses multiple methods of the iterator and is suceptible to a race between the calls to hasNext() and next(). The test in this commit fails under the old implementation with a NoSuchElementException. This commit adds a wrapper object over a set and a list, with all methods being synchronized to avoid races. Additionally, iterators are no longer used and replaced with a counter and index based access of an array list to maintain the round robin aspect of the previous node supplier implementation.
Member
Author
|
@pickypg observed the following errors that were caused by this race condition: |
s1monw
suggested changes
Jun 28, 2017
| // nodes, this class uses a counter and retrieves by index from the list. The arraylist enables us to do this in O(1). | ||
| private final Set<DiscoveryNode> nodeSet = new HashSet<>(); | ||
| private final List<DiscoveryNode> nodeList = new ArrayList<>(); | ||
| private final AtomicInteger counter = new AtomicInteger(0); |
Contributor
There was a problem hiding this comment.
this can be a simple integer now?
| // this classes uses both a set and a list to support faster operations. Insertion and contains for this class are O(1) thanks | ||
| // to the use of the set and removal is O(n) due to the arraylist. In order to support a round-robin scheme through the connected | ||
| // nodes, this class uses a counter and retrieves by index from the list. The arraylist enables us to do this in O(1). | ||
| private final Set<DiscoveryNode> nodeSet = new HashSet<>(); |
Contributor
There was a problem hiding this comment.
I don't like to have 2 data structures that we have to keep in sync. if you really wanna optimize for removal / insertion we can just use an array or an array list and sort it? I doubt we should to be honest. Maybe we just go with a list and linear scan or we use a set only and the same iterator approach and with the synchronization in place we can just set the iterator to null if we changed the set? that might be the easiest
jaymode
added a commit
that referenced
this pull request
Jun 28, 2017
This commit fixes a race condition in the node supplier used by the RemoteClusterConnection. The node supplier stores an iterator over a set backed by a ConcurrentHashMap, but the get operation of the supplier uses multiple methods of the iterator and is suceptible to a race between the calls to hasNext() and next(). The test in this commit fails under the old implementation with a NoSuchElementException. This commit adds a wrapper object over a set and a iterator, with all methods being synchronized to avoid races. Modifications to the set result in the iterator being set to null and the next retrieval creates a new iterator.
jaymode
added a commit
that referenced
this pull request
Jun 28, 2017
This commit fixes a race condition in the node supplier used by the RemoteClusterConnection. The node supplier stores an iterator over a set backed by a ConcurrentHashMap, but the get operation of the supplier uses multiple methods of the iterator and is suceptible to a race between the calls to hasNext() and next(). The test in this commit fails under the old implementation with a NoSuchElementException. This commit adds a wrapper object over a set and a iterator, with all methods being synchronized to avoid races. Modifications to the set result in the iterator being set to null and the next retrieval creates a new iterator.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes a race condition in the node supplier used by the RemoteClusterConnection. The
node supplier stores an iterator over a set backed by a ConcurrentHashMap, but the get operation
of the supplier uses multiple methods of the iterator and is suceptible to a race between the
calls to hasNext() and next(). The test in this commit fails under the old implementation with a
NoSuchElementException. This commit adds a wrapper object over a set and a list, with all methods
being synchronized to avoid races. Additionally, iterators are no longer used and replaced with a
counter and index based access of an array list to maintain the round robin aspect of the previous
node supplier implementation.