fix(state): stale pointer access could cause app crashes #121
Conversation
This file contains hidden or 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
Reviewer's GuideRoutes ReSwift subscriber callbacks onto the main actor, annotates observable and framework types with @mainactor, simplifies subscription management, and relies on ReSwift’s weak subscription cleanup to avoid stale references and crashes caused by background-thread state updates. Sequence diagram for main-actor-routed ReSwift state updatessequenceDiagram
participant BackgroundThread as Background_thread
participant Store as Store_RowndState
participant Observable as ObservableState_T
participant DispatchHelper as dispatchToMainActor
participant MainQueue as DispatchQueue_main
participant MainActor as MainActor_Task
participant SwiftUI as SwiftUI_View
BackgroundThread->>Store: dispatch(Action)
Store->>Store: reduce() RowndState
Store->>Observable: newState(state)
note over Observable: nonisolated newState(state : T)
Observable->>DispatchHelper: dispatchToMainActor(self, state, work)
DispatchHelper->>MainQueue: async { [weak self] ... }
MainQueue->>MainQueue: capture weak self
MainQueue->>MainActor: Task { work(instance, state) }
MainActor->>Observable: applyStateUpdate(state)
Observable->>Observable: update current, withAnimation
Observable-->>SwiftUI: objectDidChange.send(DidChangeSubject)
SwiftUI->>SwiftUI: re-render view on main thread
Class diagram for updated observable ReSwift state wrappersclassDiagram
class DispatchHelpers {
+dispatchToMainActor(instance, state, work)
}
class ObservableSubscription {
<<protocol>>
}
class Store {
+subscribe(select, animation) ObservableState
+subscribe(select, transform, animation) ObservableDerivedState
+subscribeThrottled(select, throttleInMs, animation) ObservableThrottledState
+subscribeThrottled(select, transform, throttleInMs, animation) ObservableDerivedThrottledState
}
class ObservableState~T~ {
<<@MainActor>>
-current : T
-selector
-animation : Animation?
-isSubscribed : Bool
+objectDidChange : PassthroughSubject~DidChangeSubject~T~~, Never~
+ObservableState(select, animation)
+subscribe() : Void
+unsubscribe() : Void
+nonisolated newState(state : T) : Void
-applyStateUpdate(state : T) : Void
}
class ObservableThrottledState~T~ {
<<@MainActor>>
-objectThrottled : PassthroughSubject~T, Never~
-cancellables : Set~AnyCancellable~
+ObservableThrottledState(select, animation, throttleInMs)
+nonisolated newState(state : T) : Void
-applyThrottledStateUpdate(state : T) : Void
}
class ObservableDerivedState~Original, Derived~ {
<<@MainActor>>
-current : Derived
-selector
-transform(original : Original) Derived
-animation : Animation?
-isSubscribed : Bool
+objectWillChange : PassthroughSubject~ChangeSubject~Derived~~, Never~
+objectDidChange : PassthroughSubject~ChangeSubject~Derived~~, Never~
+ObservableDerivedState(select, transform, animation)
+subscribe() : Void
+unsubscribe() : Void
+nonisolated newState(state : Original) : Void
-applyStateUpdate(original : Original) : Void
}
class ObservableDerivedThrottledState~Original, Derived~ {
<<@MainActor>>
-objectThrottled : PassthroughSubject~Original, Never~
-cancellables : Set~AnyCancellable~
+ObservableDerivedThrottledState(select, transform, animation, throttleInMs)
+nonisolated newState(state : Original) : Void
-applyThrottledStateUpdate(original : Original) : Void
}
class InstantUsers {
<<@MainActor>>
-context : Context
-cancellables : Set~AnyCancellable~
+InstantUsers(context : Context)
+tmpForceInstantUserConversionIfRequested() : Void
}
class RowndEventEmitter {
<<@MainActor>>
-cancellables : Set~AnyCancellable~
+emit(event : RowndEvent) : Void
}
ObservableState ..|> StoreSubscriber
ObservableState ..|> ObservableSubscription
ObservableThrottledState --|> ObservableState
ObservableDerivedState ..|> StoreSubscriber
ObservableDerivedState ..|> ObservableSubscription
ObservableDerivedThrottledState --|> ObservableDerivedState
Store --> ObservableState : creates
Store --> ObservableThrottledState : creates
Store --> ObservableDerivedState : creates
Store --> ObservableDerivedThrottledState : creates
DispatchHelpers ..> ObservableState : dispatchToMainActor
DispatchHelpers ..> ObservableThrottledState : dispatchToMainActor
DispatchHelpers ..> ObservableDerivedState : dispatchToMainActor
DispatchHelpers ..> ObservableDerivedThrottledState : dispatchToMainActor
InstantUsers ..> Store : uses
RowndEventEmitter ..> Store : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Using
Task { @MainActor ... }in the nonisolatednewStatemethods can spawn many unstructured tasks and reorder or drop intermediate state updates under high-frequency changes; consider usingawait MainActor.run { ... }(and making the method async) or some form of coalescing to preserve ordering guarantees. - The
Task { @MainActor [weak self] ... }pattern is duplicated across the observable state types; consider extracting a small helper (e.g.,func onMainActor(_ body: @MainActor @escaping (Self) -> Void)) to reduce boilerplate and keep the weak-self/thread-hopping logic consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `Task { @MainActor ... }` in the nonisolated `newState` methods can spawn many unstructured tasks and reorder or drop intermediate state updates under high-frequency changes; consider using `await MainActor.run { ... }` (and making the method async) or some form of coalescing to preserve ordering guarantees.
- The `Task { @MainActor [weak self] ... }` pattern is duplicated across the observable state types; consider extracting a small helper (e.g., `func onMainActor(_ body: @MainActor @escaping (Self) -> Void)`) to reduce boilerplate and keep the weak-self/thread-hopping logic consistent.
## Individual Comments
### Comment 1
<location> `Sources/Rownd/Rownd.swift:103` </location>
<code_context>
- }
-
- InstantUsers(context: Context.currentContext)
- .tmpForceInstantUserConversionIfRequested()
+ InstantUsers(context: Context.currentContext)
+ .tmpForceInstantUserConversionIfRequested()
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** InstantUsers conversion now runs inside the Task instead of immediately after; verify this change in timing is intentional.
Moving `InstantUsers(context: ...)` inside the `Task` changes when this side effect executes (now on the task’s async context instead of immediately after `Task` creation). Please confirm no callers depend on this conversion having started by the time `hydrate()` (or subsequent synchronous code) returns, and that this timing shift is intentional.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The documentation on
dispatchToMainActormentions serializing updates through a Combine subject, but the implementation now usesDispatchQueue.main.async+Task; consider updating the comment to accurately describe the current behavior. - In
ObservableThrottledState.applyThrottledStateUpdateandObservableDerivedThrottledState.applyThrottledStateUpdate, theoldlocal is computed but never used; either remove it or wire it into the change notifications if you intended to include the previous value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The documentation on `dispatchToMainActor` mentions serializing updates through a Combine subject, but the implementation now uses `DispatchQueue.main.async` + `Task`; consider updating the comment to accurately describe the current behavior.
- In `ObservableThrottledState.applyThrottledStateUpdate` and `ObservableDerivedThrottledState.applyThrottledStateUpdate`, the `old` local is computed but never used; either remove it or wire it into the change notifications if you intended to include the previous value.
## Individual Comments
### Comment 1
<location> `Sources/Rownd/Models/Context/ReSwiftObserver.swift:17-26` </location>
<code_context>
+
+/// Dispatches work to the MainActor from a nonisolated context.
+///
+/// This helper serializes state updates through a Combine subject to maintain FIFO ordering,
+/// then processes them on the MainActor. This prevents the ordering issues that can occur
+/// with unstructured Task spawning under high-frequency state changes.
+///
+/// - Parameters:
+/// - instance: The object to operate on (captured weakly)
+/// - state: The state value to process
+/// - work: The MainActor-isolated work to perform
+private func dispatchToMainActor<T: AnyObject, S>(
+ _ instance: T,
+ state: S,
+ work: @escaping @MainActor (T, S) -> Void
+) {
+ // Use DispatchQueue.main.async for FIFO ordering, then Task for @MainActor isolation
DispatchQueue.main.async { [weak instance] in
guard let instance = instance else { return }
</code_context>
<issue_to_address>
**issue:** Doc comment for dispatchToMainActor no longer matches its implementation.
The comment still refers to serializing updates via a Combine subject, but the code now uses `DispatchQueue.main.async` followed by a `Task { @MainActor in ... }` with no Combine involvement. Please update the doc to describe the actual behavior (main queue FIFO + MainActor hop) so future readers can accurately reason about ordering guarantees.
</issue_to_address>
### Comment 2
<location> `Sources/Rownd/Rownd.swift:100` </location>
<code_context>
}
- }
- InstantUsers(context: Context.currentContext)
- .tmpForceInstantUserConversionIfRequested()
+ InstantUsers(context: Context.currentContext)
+ .tmpForceInstantUserConversionIfRequested()
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** InstantUsers side-effect is now conditioned on store existence, changing behavior.
`tmpForceInstantUserConversionIfRequested()` used to run regardless of whether `store` existed; now it only runs when `Context.currentContext.store` is non-nil. If this logic should apply even when the store isn’t configured yet, this may unintentionally skip conversions. If the dependency on `store` is intentional, please document it; otherwise, consider moving this call back outside the `if let` or making the dependency explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+17
to
+26
| /// This helper serializes state updates through a Combine subject to maintain FIFO ordering, | ||
| /// then processes them on the MainActor. This prevents the ordering issues that can occur | ||
| /// with unstructured Task spawning under high-frequency state changes. | ||
| /// | ||
| /// - Parameters: | ||
| /// - instance: The object to operate on (captured weakly) | ||
| /// - state: The state value to process | ||
| /// - work: The MainActor-isolated work to perform | ||
| private func dispatchToMainActor<T: AnyObject, S>( | ||
| _ instance: T, |
There was a problem hiding this comment.
issue: Doc comment for dispatchToMainActor no longer matches its implementation.
The comment still refers to serializing updates via a Combine subject, but the code now uses DispatchQueue.main.async followed by a Task { @MainActor in ... } with no Combine involvement. Please update the doc to describe the actual behavior (main queue FIFO + MainActor hop) so future readers can accurately reason about ordering guarantees.
Contributor
Author
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
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.
Summary by Sourcery
Ensure ReSwift observable state updates and related framework components are safely marshalled to the main actor to prevent crashes from background-thread state notifications.
Bug Fixes:
Enhancements:
Tests: