Crash when writing to user defaults in background thread

[lukeredpath]

Description

After updating a feature to use @Shared with the .appStorage persistence key, our app has started crashing when entering the background.

Checklist

• [X] I have determined whether this bug is also reproducible in a vanilla SwiftUI project.
• [X] If possible, I've reproduced the issue using the main branch of this package.
• [X] This issue hasn't been addressed in an existing GitHub issue or discussion.

Expected behavior

View updates are triggered on the main actor and the app does not crash.

Actual behavior

The app crashes when user defaults are updated from a background thread.

Steps to reproduce

I have traced this back to a custom view property wrapper that implements DynamicProperty - its update() method is marked as nonisolated to fix a concurrency warning but uses MainActor.assumeIsolated in this function because this should only be called on the main actor as part of a view update, however because the view is updating on a background thread it crashes.

The background view update is being triggered by a third party SDK that writes to the user defaults when the app enters the background. This in turns triggers the NotificationCenter observer in the app storage property key on the same background thread, which calls didSet.

I updated TCA to point to main as I thought #3206 would fix this issue however the final fix in that PR did not make the entire willSet isolated to the main actor, only the part the updates the current value but as you can see from the backtrace above, the view update is triggered before that (or possibly, IIRC the update() function on a DynamicProperty is called before the view is about to be recomputed which might be why its crashing when calling willSet on the registrar).

I think the entire didSet closure needs to be called on the main actor, so mainActorASAP should wrap the entire closure contents.

Alternatively (or additionally), would it make more sense to ensure that all notifications are received on the main queue in the first place by passing .main to the queue when calling NotificationCenter.default.addObserver?

The Composable Architecture version information

54eb417336fa849270671b8d2b73180835f9975a

Destination operating system

iOS 17

Xcode version information

Xcode 15.3

Swift Compiler version information

swift-driver version: 1.90.11.1 Apple Swift version 5.10 (swiftlang-5.10.0.13 clang-1500.3.9.4)
Target: arm64-apple-macosx14.0

[lukeredpath]

This fix worked for me locally: https://github.com/pointfreeco/swift-composable-architecture/compare/main...lukeredpath:swift-composable-architecture:lukeredpath/will-set-crash

FWIW, both of these solutions work in isolation - I've tested with both the notification queue fix and the change to mainActorASAP, I'm not sure if you want to make both or one of those changes.

[larryonoff]

The same issue for me. See screens with the details below.

The problem occurs for AppStoreKey when someone updates UserDefaults from the thread other than main, so that _$perceptionRegistrar is called from the thread other than main.

          self._$perceptionRegistrar.willSet(self, keyPath: \.value)
          defer { self._$perceptionRegistrar.didSet(self, keyPath: \.value) }

[stephencelis]

@lukeredpath With the mainActorASAP, is the .main queue necessary? I would think we would only need the former.

Also we're definitely down to take a PR for this! Any chance you have time to do so and maybe add a quick test that would have failed before the fix?

[lukeredpath]

Yes I will see what I can do. I think only one fix is necessary but I'm not sure which is best.

[stephencelis]

I think mainActorASAP will avoid a thread hop if already on the main thread, so maybe just that one?

[larryonoff]

@lukeredpath Will you be able to proceed with the fix? I can take it, if - not. We still have crashes in production, so It would be great to include the fix in upcoming TCA 1.13.0.

I would leave mainActorASAP for now.

[larryonoff]

I decided not to wait and created PR. @stephencelis could you please include the fix in upcoming release.

[lukeredpath]

@larryonoff thank you, I was unable to get a reproducing test - I was using observe {} which takes care of shunting on to the main thread for you so it wasn't working.

[larryonoff]

@lukeredpath Hope the issue has finally been resolved. It has been in production since we began using Shared.

[lukeredpath]

Fixed by #3247

[zachwaugh]

I'm still seeing a crash here in 1.13.1. I'm not seeing anything obvious as a cause but the one thing that did stick out was this was during a fresh install of the app, so I think it's related to the case when the user default doesn't have an existing value.

[stephencelis]

@zachwaugh This looks like it's a different issue than the one folks were encountering here. Can you share a repro and more information in a new issue/discussion? For example, it looks like SwiftUI is invoking a precondition in the attribute graph, so I would hope there'd be helpful info printed to the console. Can you report back if so?