fix: Deadlock issue caused in listResources (argoproj/argo-cd#18902) #599
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
It is possible for Argo to reach a state where the maximum amount of go-routines have incremented the semaphore and are waiting on the lock in loadInitialState WHILE another go-routine is holding the lock waiting on the semaphore. This change allows the go-routine holding the lock to bypass the semaphore and complete its execution. The downside of this approach is that more could be running than intended, but that is preferable to the cache deadlocking. Signed-off-by: James Root <jroot@indeed.com>
crenshaw-dev
reviewed
Jul 5, 2024
…/argo-cd#18902) Signed-off-by: James Root <jroot@indeed.com>
agaudreault
reviewed
Jul 11, 2024
| func (c *clusterCache) listResources(ctx context.Context, resClient dynamic.ResourceInterface, callback func(*pager.ListPager) error) (string, error) { | ||
| if err := c.listSemaphore.Acquire(ctx, 1); err != nil { | ||
| return "", err | ||
| func (c *clusterCache) listResources(ctx context.Context, resClient dynamic.ResourceInterface, takeSemaphore bool, callback func(*pager.ListPager) error) (string, error) { |
There was a problem hiding this comment.
I was working on the "same" issue. I had the idea to skip the semaphore, but while it would solve the problem, it disables the feature added by 605958d
See PR suggestion #604.
Both PRs solve the issue, let's discuss in this thread what option is the best one. @gdsoumya you added the replaceResourceCache as part of the listResource callback (#532). Any specific reason?
|
Superseded by #604. If memory consumption is too unpredictible with the change, let revisit and check if we allow a mechanism to skip the semaphore 👍 Good job on finding the deadlock nontheless, I wish I had saw your issue sooner! |
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.
Checklist:
Fixes argoproj/argo-cd#18902