Conversation
duyhungtnn
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces thread safety improvements to the InMemoryCache component and adds comprehensive concurrency tests to verify thread-safe behavior of the evaluation storage system. The changes aim to prevent data races during concurrent read/write operations.
Key changes:
- Refactored
InMemoryCacheto use a concurrentDispatchQueuewith barrier writes for thread-safe cache operations - Added new test suite
EvaluationStorageImplConcurrencyTestswith concurrency stress tests and read-during-write performance tests - Updated existing E2E tests to properly synchronize evaluation storage access using the client's dispatch queue
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
Bucketeer/Sources/Internal/Evaluation/EvaluationMemCacheDao.swift |
Implements concurrent queue with barrier writes and concurrent reads for thread-safe cache operations |
BucketeerTests/EvaluationStorageImplConcurrencyTests.swift |
Adds comprehensive concurrency tests including stress tests and non-blocking read verification with a custom BlockingRealSQLDao helper |
BucketeerTests/E2E/E2EBKTClientForceUpdateTests.swift |
Wraps evaluation storage access in dispatchQueue.sync blocks to ensure thread-safe test assertions |
BucketeerTests/E2E/E2EEventTests.swift |
Minor formatting/indentation corrections in test assertions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bucketeer/Sources/Internal/Evaluation/EvaluationMemCacheDao.swift
Outdated
Show resolved
Hide resolved
Bucketeer/Sources/Internal/Evaluation/EvaluationMemCacheDao.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
duyhungtnn
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bucketeer/Sources/Internal/Evaluation/EvaluationStorageImpl.swift
Outdated
Show resolved
Hide resolved
Bucketeer/Sources/Internal/Evaluation/EvaluationStorageImpl.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bucketeer/Sources/Internal/Evaluation/EvaluationInteractor.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bucketeer/Sources/Internal/Evaluation/EvaluationStorageImpl.swift
Outdated
Show resolved
Hide resolved
Bucketeer/Sources/Internal/Evaluation/EvaluationStorageImpl.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Bucketeer/Sources/Internal/Evaluation/EvaluationStorageImpl.swift
Outdated
Show resolved
Hide resolved
Updated comments in InMemoryCache to clarify concurrency behavior for set and get methods. Enhanced BlockingRealSQLDao in tests to assert expectation wait results, improving error reporting for failed waits. Removed unused helper from test file.
Introduced tests to verify thread safety and concurrent access behavior of InMemoryCache. Also fixed a typo in E2EBKTClientForceUpdateTests and improved a comment in EvaluationStorageImplConcurrencyTests for clarity.
Added detailed comments to clarify the thread-safety and design decisions behind accessing the memory cache directly in the getBy(featureId:) method. The comments explain the trade-off between responsiveness and data consistency, and why strict locking is avoided to prevent UI freezes.
The setUserAttributesUpdated call is now executed within an execute block to ensure proper handling, possibly for thread safety or sequencing.
Refactored updateUserAttributes to call setUserAttributesUpdated directly, removing the execute wrapper for this call.
Replaces the boolean userAttributesUpdated flag with a version number to prevent race conditions where concurrent updates could be lost during evaluation fetches. Updates EvaluationStorage protocol and implementations to track and clear the flag only when the version matches, ensuring no updates are missed. Adds corresponding unit tests to verify correct behavior.
Introduces a test to verify thread safety of setUserAttributesUpdated and related methods in EvaluationStorageImpl by simulating concurrent updates and clears from different queues. Ensures no race conditions occur and the version increments as expected.
Replaces the mock user defaults DAO with the real EvaluationUserDefaultDaoImpl to ensure thread-safe access during concurrent operations. Increases the number of iterations to 10,000 for more robust concurrency testing and clarifies test assertions and comments.
Renamed userAttributesUpdatedLock to setUserAttributesUpdatedLock and ensured all accesses to userAttributesUpdatedVersion are protected by the lock. Refactored setUserAttributesUpdated and clearUserAttributesUpdated to use the new locking mechanism, improving thread safety in tests.
Added comments to clarify that userAttributesUpdatedVersion increments on setUserAttributesUpdated, and expanded documentation on thread safety and access patterns in getBy for EvaluationStorageImpl.
Reorders async operations in EvaluationStorageTests to reduce race conditions and clarifies the expected outcome. Removes the assertion on userAttributesUpdated's final state, focusing instead on the version counter to verify correct behavior.
Replaces separate userAttributesUpdated and userAttributesUpdatedVersion properties with a unified UserAttributesState struct in EvaluationStorage and related implementations. Updates all usages and tests to use the new struct, improving atomicity and clarity of user attribute update tracking.
Updated the documentation for UserAttributesState to clarify its intended use as an in-memory snapshot for a single app session and to specify the persistence behavior of its properties.
Replaces the userAttributesUpdated property with userAttributesState.isUpdated in EvaluationStorage and related tests. This change improves clarity by using the UserAttributesState struct for tracking updates, and updates all references in implementation and tests accordingly.
Added a comment explaining the purpose of the userAttributesUpdatedVersion variable and its thread safety mechanism in EvaluationStorageImpl.
Replaces version-based clearing of the user-attributes-updated flag with a method that accepts a UserAttributesState snapshot, improving atomicity and thread-safety. Updates EvaluationStorage protocol, its implementation, and related tests and mocks to use the new approach.
Introduces a deleteAll() method to EvaluationUserDefaultDaoImpl for removing all related UserDefaults keys, and updates E2E tests to use this method for setup and teardown. Adds removeObject to Defaults protocol and MockDefaults, and makes minor improvements to MockEvaluationStorage and test concurrency handling.
Deleted a detailed comment describing a logical race condition with the userAttributesUpdated boolean and a proposed solution. The comment is no longer needed, possibly due to code changes or refactoring.
Refactored MockDefaults to use a concurrent DispatchQueue for thread-safe access to the underlying dictionary. Read operations are now concurrent, while write operations use barrier flags to ensure exclusive access and data consistency.
Added detailed documentation to UserAttributesState explaining the ephemeral version counter and optimistic locking pattern. Removed unused userAttributesUpdated property and redundant dict property in MockDefaults for code clarity.
Improved documentation in EvaluationStorage and its tests to clarify the behavior of the version counter and update flag, especially regarding race conditions and expected outcomes during concurrent operations.
Introduced EvaluationUserDefaultsKey enum to centralize user defaults keys and updated references throughout the codebase. Added UserDefaults.removeAllEvaluationData() helper for test cleanup. Updated tests and mocks to use new key management and helper method, improving maintainability and reducing duplication.
duyhungtnn
force-pushed
the
feat/improve-read-evaluation-threadsafe
branch
from
db1e644 to
dcd7cd1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR introduces thread-safe user attribute update tracking and improves overall concurrency handling in the evaluation system.
Main changes:
Fixed a logical race condition related to userAttributesUpdated by introducing a versioned User Attributes State.
The boolean userAttributesUpdated flag is replaced with a UserAttributesState struct containing a version counter and an update flag. This prevents race conditions where concurrent attribute updates could be lost during in-flight fetch operations. The clearing logic now uses an atomic compare-and-swap based on version matching.
Fixed a potential race condition when reading evaluations that could cause a runtime crash.
This is addressed by implementing a thread-safe InMemoryCache using a concurrent queue with barrier synchronization, ensuring safe reads and writes without blocking the main thread.
Test infrastructure for fix some flaky tests fail