Synchronize calls to DiscoverPollEndpoint #4504
Open
+108
−17
Conversation
This file contains 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
tshan2001
previously approved these changes
Feb 17, 2025
timj-hh
reviewed
Feb 18, 2025
xxx0624
reviewed
Feb 19, 2025
isaac-400
force-pushed
the
sync-dpe
branch
3 times, most recently
from
556f971 to
870c60e
Compare
isaac-400
force-pushed
the
sync-dpe
branch
2 times, most recently
from
d5aa82e to
105da09
Compare
added 2 commits
February 20, 2025 09:36
Agents that share ECSClients can call DiscoverPollEndpoint (DPE) multiple times per task. Each routine that calls DPE will first check the cache before performing the actual API call over the network. The intention here is that only one actual API call is performed (by the first routine to call DPE). However, it is possible for multiple routines to race and effectively make many actual API calls. This is because the `pollEndpointCache` is only updated when the first API call _returns_. This change enforces the intended behavior by making subsequent routines wait for the cache to be updated (or not) by the first thread, eliminating simultaneous calls to DPE.
xxx0624
approved these changes
Feb 20, 2025
sparrc
approved these changes
Feb 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Agents that share ECSClients can call DiscoverPollEndpoint (DPE) multiple times per task. Each routine that calls DPE will first check the cache before performing the actual API call over the network. The intention here is that only one actual API call is performed (by the first routine to call DPE).
However, it is possible for multiple routines to race and effectively make many actual API calls. This is because the
pollEndpointCacheis only updated when the first API call returns.This change enforces the intended behavior by making subsequent goroutines wait for the cache to be updated (or not) by the first goroutine, eliminating simultaneous calls to DPE.
Implementation details
The implementation adds a lock to the ECSClient implementation of DiscoverPollEndpoint, thus concurrent calls to DPE will execute one at a time.
Since other DiscoverPollEndpointCalls will block, this change also adds a conservative timeout for the method (AWS Blog Reference)
Testing
make testNew tests cover the changes: yes
Description for the changelog
* Bug - Fixed a race condition with concurrent DiscoverPollEndpoint callsAdditional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
No
Does this PR include the addition of new environment variables in the README?
No
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.