fix: concurrent SlugifyName access (#18369)#18370
fix: concurrent SlugifyName access (#18369)#18370mhotan wants to merge 1 commit intoargoproj:masterfrom
Conversation
Signed-off-by: Mike Hotan <mike@union.ai>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #18370 +/- ##
=======================================
Coverage 45.68% 45.68%
=======================================
Files 353 353
Lines 46965 46967 +2
=======================================
+ Hits 21454 21456 +2
Misses 22772 22772
Partials 2739 2739 ☔ View full report in Codecov by Sentry. |
|
Ideally, this simple logic should not require being gated by a Mutex. That being said, I doubt we will be able to update the underlying library. Two PRs have been stale for at least three years.
If we are not comfortable using a Mutex we should probably update the documentation indicating Slugify is not multi-thread safe. |
andrii-korotkov-verkada
left a comment
There was a problem hiding this comment.
Note: you need to specify # before the issue number in the PR description for the bot to actually close it.
| 1. [UFirstGroup](https://www.ufirstgroup.com/en/) | ||
| 1. [ungleich.ch](https://ungleich.ch/) | ||
| 1. [Unifonic Inc](https://www.unifonic.com/) | ||
| 1. [Union Inc](https://www.union.ai/) |
There was a problem hiding this comment.
Let's have a separate PR for this
| // Necessary mutex due to global shared state. | ||
| // https://github.com/argoproj/argo-cd/issues/18369 | ||
| slugifyNameMutex.Lock() | ||
| defer slugifyNameMutex.Unlock() |
There was a problem hiding this comment.
Seems like we can defer locking to below the loop
Fixes 18369
Checklist: