[MBL-19677][S/P/T] Structured Concurrency foundations #3852
Conversation
refs: MBL-19677 builds: Student, Teacher, Parent affects: Student, Teacher, Parent release note: none
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR introduces a new AsyncStore implementation that provides async/await alternatives to the existing reactive Store pattern. The changes align well with Swift's modern concurrency model and follow the project's MVVM architecture.
Issues Found
-
Critical Bug in NSPersistentContainerExtensions.swift:99-103 - The
performWriteTaskmethods are usingself.contextinstead of the computedcontextproperty, which defeats the purpose of having a dedicated write context. This could lead to threading issues. -
Memory Leak Risk in AsyncStore.swift:139 - The
streamEntitiesFromDatabase()method returns a non-terminating stream without built-in cancellation handling. While the warning comment is present, consider adding safeguards or helper methods. -
Silent Error Handling in AsyncStore.swift:147 - The
forceRefreshmethod silently ignores errors withtry?. This should at least log errors for debugging purposes. -
SwiftLint Violation in AsyncStore.swift:54 - Line length exceeds the project's limit and should be broken into multiple lines.
Positive Aspects
-
Clean async/await API: The new
AsyncStoreprovides a modern, ergonomic interface that will improve code readability compared to Combine publishers. -
Offline mode support: Proper integration with
OfflineModeInteractorensures consistent behavior across online/offline states. -
Smart caching: The cache validation logic is well-implemented with
hasCacheExpiredchecks before making API calls. -
Consistent error handling: Most methods properly propagate errors through
async throws, giving callers control over error handling. -
Pagination support: The
loadAllPagesparameter provides flexibility for different data loading scenarios. -
Good separation of concerns: Private helper methods like
fetchEntitiesFromCache,fetchEntitiesFromAPI, andfetchEntitiesFromDatabasekeep the code organized and testable.
Recommendations
-
Fix the critical bug: The
performWriteTaskimplementation must use the write context, not the view context. -
Add unit tests: Given the complexity of async streams and Core Data operations, comprehensive unit tests are essential. Follow the conventions in CLAUDE-unit-tests.md.
-
Consider adding cancellation helpers: For non-terminating streams, provide convenience methods that handle cancellation automatically (e.g., with timeouts or lifecycle-bound cancellation).
-
Add logging: At minimum, log errors in
forceRefreshto aid debugging. -
SwiftLint compliance: Ensure all code passes SwiftLint checks before merging.
| public func getEntities(ignoreCache: Bool = false, loadAllPages: Bool = true) async throws -> [U.Model] { | ||
| let scope = useCase.scope | ||
| let request = NSFetchRequest<U.Model>(entityName: String(describing: U.Model.self)) | ||
| request.predicate = scope.predicate |
There was a problem hiding this comment.
SwiftLint violation: This line exceeds the line length limit. Consider breaking it into multiple lines for better readability.
|
|
||
| /// - Warning: This stream **does not terminate**. Ensure proper cancellation of its consuming task. | ||
| public func streamEntitiesFromDatabase() throws -> AsyncThrowingStream<[U.Model], Error> { | ||
| let scope = useCase.scope |
There was a problem hiding this comment.
Critical: This stream does not terminate and can cause memory leaks if not properly cancelled. Consider:
- Adding a timeout mechanism
- Providing a helper method that handles cancellation
- Adding unit tests that verify proper cleanup when the consuming task is cancelled
| return Self.streamEntitiesFromDatabase(fetchRequest: request, context: context) | ||
| } | ||
|
|
||
| /// Refreshes the entities by requesting the latest data from the API. |
There was a problem hiding this comment.
The error from forceRefresh is being silently ignored with try?. This could hide important issues. Consider:
- Logging the error for debugging
- Using a completion handler or throwing the error to the caller
- At minimum, adding a comment explaining why it's safe to ignore errors here
|
|
||
| @objc open func performWriteTask(_ block: @escaping (NSManagedObjectContext) -> Void) { | ||
| let context = writeContext ?? { | ||
| context.perform { block(self.context) } |
There was a problem hiding this comment.
Bug: This implementation is incorrect. The method is supposed to use a write context, but it's always using self.context instead of the actual context variable returned by the context computed property.
The block(self.context) should be block(context) where context is assigned from the context computed property.
| } | ||
|
|
||
| public func performWriteTask<T>(_ block: @escaping (NSManagedObjectContext) throws -> T) async rethrows -> T { | ||
| return try await context.perform { try block(self.context) } |
There was a problem hiding this comment.
Bug: Same issue here - this should use the write context from the context computed property, not self.context. The correct implementation should be:
let writeContext = context
return try await writeContext.perform { try block(writeContext) }
petkybenedek
changed the title
BuildsCommit: Remove newline (666c444)
|
What's new
This PR introduces AsyncStore, which makes use of structured concurrency to interact with the API and Core Data.
refs: MBL-19677
builds: Student, Teacher, Parent
affects: Student, Teacher, Parent
release note: none
Checklist