-
Notifications
You must be signed in to change notification settings - Fork 53
fix: improve thread safety in CrowdinLocalizationDownloader completion handling #346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
serhii-londar
commented
Sep 13, 2025
- Added locking mechanism to safely read accumulated state (strings, plurals, errors) when no files are available for download.
- Ensured completion handler returns the correct state without risking data corruption during concurrent access.
…n handling - Added locking mechanism to safely read accumulated state (strings, plurals, errors) when no files are available for download. - Ensured completion handler returns the correct state without risking data corruption during concurrent access.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #346 +/- ##
==========================================
+ Coverage 62.72% 63.17% +0.45%
==========================================
Files 126 126
Lines 4839 4851 +12
==========================================
+ Hits 3035 3064 +29
+ Misses 1804 1787 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances thread safety in the CrowdinLocalizationDownloader by adding proper locking when reading accumulated state in the no-files-to-download scenario. It also improves test isolation by adding comprehensive state cleanup in test setup.
- Added lock/unlock mechanism to safely capture strings, plurals, and errors before invoking completion handler
- Enhanced test setup with global state cleanup to prevent test interference
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Sources/CrowdinSDK/Providers/Crowdin/LocalizationDownloader/CrowdinLocalizationDownloader.swift | Added thread-safe state reading with lock mechanism when no files are available for download |
| Sources/Tests/Core/RealLocalizationProviderTests.swift | Added global state cleanup in setUp() and manifestManager.clear() call to improve test isolation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| self.lock.lock() | ||
| let strings = self.strings | ||
| let plurals = self.plurals | ||
| let errors = self.errors | ||
| self.lock.unlock() |
Copilot
AI
Oct 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lock is not released if an exception occurs between lock() and unlock(). Consider using defer to ensure the lock is always unlocked, even in exceptional cases: self.lock.lock(); defer { self.lock.unlock() }