[Test] Wait all results to be gathered before assertion #134279
Conversation
The assertion iterates through the list of responses. It needs to wait for the last element to be added to the list otherwise it may encounter ConcurrentModificationException Resolves: elastic#134277
ywangd
added
>test
elasticsearchmachine
added
the
Team:Distributed Coordination
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a concurrency issue in the TransportNodesActionTests.testConcurrentlyCompletionAndCancellation test method that was causing ConcurrentModificationException errors. The test was failing because it was iterating through a list of responses while another thread was still adding elements to it.
- Added synchronization using
CountDownLatchto ensure all responses are gathered before assertion - Removed the test from the muted tests list as the concurrency issue is now resolved
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| TransportNodesActionTests.java | Added CountDownLatch synchronization to wait for response completion before asserting |
| muted-tests.yml | Removed the previously muted test since the concurrency issue is fixed |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
There was a problem hiding this comment.
Hmm I think this excludes some interleavings that matter. It should be that all the responses apart from the last one are released before onCancelledLatch is triggered, regardless of whether we've received & processed all the responses, whereas now we're delaying that check until we've definitely received all the responses.
Could we use nodeResponses just for the first N-1 responses, check that they're all released on cancel, and then handle the racy Nth response separately?
|
Also hmm if we do this do we need |
|
Yeah you are right on both points. I pushed 87704ae for them. Thanks a lot! |
|
Ah yes. I changed to use PlainActionFuture see fdf51b8 |
ywangd
added
the
auto-merge-without-approval
💔 Backport failed
You can use sqren/backport to manually backport by running |
The assertion iterates through the list of responses. It needs to wait for the last element to be added to the list otherwise it may encounter ConcurrentModificationException Resolves: elastic#134277 (cherry picked from commit b03e1d4) # Conflicts: # muted-tests.yml
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
The assertion iterates through the list of responses. It needs to wait for the last element to be added to the list otherwise it may encounter ConcurrentModificationException Resolves: elastic#134277 (cherry picked from commit b03e1d4) # Conflicts: # muted-tests.yml
…) (elastic#134424) The assertion iterates through the list of responses. It needs to wait for the last element to be added to the list otherwise it may encounter ConcurrentModificationException Resolves: elastic#134277 (cherry picked from commit b03e1d4) # Conflicts: # muted-tests.yml
…34422) The assertion iterates through the list of responses. It needs to wait for the last element to be added to the list otherwise it may encounter ConcurrentModificationException Resolves: #134277 (cherry picked from commit b03e1d4) # Conflicts: # muted-tests.yml Co-authored-by: Elastic Machine <[email protected]>
…) (elastic#134424) The assertion iterates through the list of responses. It needs to wait for the last element to be added to the list otherwise it may encounter ConcurrentModificationException Resolves: elastic#134277 (cherry picked from commit b03e1d4) # Conflicts: # muted-tests.yml
3 participants
The assertion iterates through the list of responses. It needs to wait for the last element to be added to the list otherwise it may encounter ConcurrentModificationException
Resolves: #134277