Main thread checks on iOS 15 - unexpected result

[lukeredpath]

Describe the bug
I'm currently doing some smoke testing of our app on iOS 15 using the latest Xcode beta (beta 5) in preparation for the forthcoming final release.

We're using TCA 0.27 and it looks like the recently added main thread checks aren't working as expected, I'm getting output like this:

Warning:

The store was interacted with on a thread that is different from the thread the store was created on:

"ViewStore.send([redacted action])" was called on the wrong thread. Make sure that "ViewStore.send" is always called on the initial thread.

  Initial thread: <NSThread: 0x600000162680>{number = 29, name = main}
  Current thread: <_NSMainThread: 0x6000001b40b0>{number = 1, name = main}

The "Store" class is not thread-safe, and so all interactions with an instance of "Store" (including all of its scopes and derived view stores) must be done on the same thread.
---

Caught debug breakpoint. Type "continue" ("c") to resume execution.

The weird thing here is I know that the first action sent to my store would have been on the main thread because we fire an action in application(_:didFinishLaunching) so I don't know how the initial thread is not the main thread (although strangely it has the name "main").

To Reproduce
If I can put together a reproducer I will uploaded it here.

Expected behavior
When the first action in the app is sent on the main thread, other actions sent from the main thread should not trigger this breakpoint.

Environment

• TCA 0..27
• Xcode 13b5
• iOS 15.

[mbrandonw]

Very interesting, thanks for the report!

The weird thing here is I know that the first action sent to my store would have been on the main thread because we fire an action in application(_:didFinishLaunching) so I don't know how the initial thread is not the main thread (although strangely it has the name "main").

The "Initial thread" referenced in that message refers to the thread where the root store was created on. Could you breakpoint where that happens and confirm its on the main thread? And that Thread.current is the main thread?

If it really is possible to have two different thread references that both represent the "main" thread then we may have to rethink some of this.

[lukeredpath]

Hmm, interesting. So our root store is created by a coordinator object that is initialised as a singleton, (e.g. static let liveCoordinator = Coordinator()) - the store is created in the Coordinator's init.

We first reference this live coordinator in the application(_:didFinishLaunching) method although due to the static I'm not 100% of the lifecycle.

IIRC, "thread 29" appeared to be owned by NSURLConnection - I don't know if that provides a clue. We do reference our app coordinator in application(_:handleEventsForBackgroundSession:completionHandler:) too but I'd be surprised if this method was even called in my very limited simulator smoke testing (I hadn't even backgrounded the app). Very odd that this thread had the name "main" though. I've always assumed the main thread is always Thread 1.

I will keep an eye on this and see if it happens again as we continue to test. I've only experienced it once so far. It doesn't help that we are still running a more than month old beta. 🤞🏻 for an updated Xcode this week.

[mbrandonw]

One thing you could try is to temporarily change the static let liveCoordinator to be a static var computed property so that you can get a breakpoint in there and confirm the thread.

[tgrapperon]

@lukeredpath Is the app using an "old" UIApplicationDelegate? Are you using some kind of logging/crash reporting framework? I've seen these things mess with NSObject.load() in the past, so I can imagine them creating additional threads and bootstrapping the store by mistake.

[lukeredpath]

@tgrapperon we've actually just converted our app over to the SwiftUI app lifecycle but we are still using @UIApplicationDelegateAdaptor and our old app delegate class.

We do have a few analytics/crash reporting tools too that could potentially be messing with this. I'll give Brandon's suggestion a try, but most of the time it seems to work OK. I might add a precondition check to our Coordinator.init method and guard that it is initialised on the main thread.

[stephencelis]

@lukeredpath Have you managed to find the root of the problem? Was the store somehow being initialized on a background thread? We'd love to know if this is an issue with our thread checking code or not 😄

[lukeredpath]

@stephencelis not yet - we'll be running our app on iOS 15 more extensively over the next week now the Xcode RC has dropped, if this wasn't a one-off issue I'm sure we'll see it again, so I'll keep an eye out. I'm 99.9% certain that our store is not being created on a background thread though.

[lukeredpath]

No sooner had I posted, another member of my team just experienced this bug - except this was on Xcode 12.5.1 on iOS 14.8.

Make sure that "ViewStore.send" is always called on the initial thread.

  Initial thread: <NSThread: 0x281df7880>{number = 10, name = main}
  Current thread: <NSThread: 0x281dac9c0>{number = 1, name = main}

The "Store" class is not thread-safe, and so all interactions with an instance of "Store" (including all of its scopes and derived view stores) must be done on the same thread.

Hmm....

[tgrapperon]

@lukeredpath If you create a dedicated init for Store and use it for your root store in your Coordinator, you can easily check which thread is running this code.

[lukeredpath]

I'm going with this for now, in the function that creates our root store:

if !Thread.isMainThread {
  assertionFailure("The main TCA store must be created on the main thread.")
}

If that never asserts and we still hit this issue, then there must be something else going on here.

[lukeredpath]

Right, looks like we can still hit this problem without the above assertion being hit which points to the underlying Thread object that represents the main thread changing - I'm just going to pair with my colleague and see if we can get more info.

[mbrandonw]

I'd be interested in seeing what Thread.isMainThread prints when the store is created and when the warning breakpoint is triggered. I'm wondering if thread identity can't be relied on for the main thread, but maybe isMainThread can be relied on.

We're also experimenting with using thread variables to detect this situation. I would hope that even if there are two different references for the main thread that they would at least still share the same thread variables.

[mbrandonw]

@lukeredpath Could you point your project to this branch and see if you can reproduce?

https://github.com/pointfreeco/swift-composable-architecture/tree/thread-identity

[iampatbrown]

@mbrandonw would Thread.current.threadDictionary[ObjectIdentifier(self)] = () work as well?
edit: Never mind :P Finished reading the whole discussion and saw #803

[lukeredpath]

Right, I think we've tracked down the problem - we were creating a scoped store on a background thread. We have a feature where we prompt for access to the user's contacts and the API for requesting permission calls back on a non-main thread - this is where we were initialising a scoped store to inject into our view. This is also why we didn't see it every time - once you've already granted permission once, subsequent calls to this API just call back immediately on the same thread.

So the bug was in our code after all (phew) but its very strange that it is being printed with a name main.

We set a breakpoint on the code that calls .scope and verified that we were on a background thread (one of the Dispatch global queues in fact):

(lldb) po Thread.current
<NSThread: 0x2835cd800>{number = 5, name = (null)}

The weird thing is that when TCA prints its warning message, the name has changed:

  Initial thread: <NSThread: 0x2835cd800>{number = 5, name = main}
  Current thread: <NSThread: 0x2835c8680>{number = 1, name = main}

Anyway, I think we can close this but I do wonder if TCA could also surface a similar warning/breakpoint if you try to call .scope on a Store from a different thread to the parent store's initial thread. WDYT?

[mbrandonw]

Ah glad you tracked it down! Thanks for digging into all of that.

Anyway, I think we can close this but I do wonder if TCA could also surface a similar warning/breakpoint if you try to call .scope on a Store from a different thread to the parent store's initial thread. WDYT?

Totally agree, we'll PR something soon.

[tgrapperon]

Maybe TCA could ship with a Main Thread locked subclass of Store? If this class is put on the Main Actor, we would furthermore get compile time checks that calls are realized on one unique thread. I don't know if Effects could be isolated too.

[tgrapperon]

It seems one can't isolate overridden non-isolated methods, so this looks like a dead end, at least in this form.

[stephencelis]

@tgrapperon Having a main actor store is definitely on our (ever-growing) list of things to look into. We have some vague ideas around it, but will take some experimentation.

[mbrandonw]

Also I'm going to convert this to a discussion for now. May help others in the future.

[mbrandonw]

@lukeredpath Hopefully this will help in the future! #803

[mluisbrown]

@mbrandonw and @lukeredpath

I'm seeing a related issue, also caused by the thread checking code:

In our app we have a number of "Services" which are independent TCA Stores and run on a background thread, they are not part of the main App State. We create a separate DispatchQueue for running these services with DispatchQueue(label: "com.trading.CoreServices.queue"). However, we create these Stores on the main queue.

I was trying to find a workaround in our code for this, by creating the stores using DispatchQueue.sync or DispatchQueue.async (using our services queue), but that also doesn't work since DispatchQueue.sync will often optimize to run the block on the current thread anyway, and DispatchQueue.async will not always run blocks for the same queue on the same thread, as dispatch queues use a thread pool.

This means it's actually impossible to guarantee that a Store is created on the same thread on which send(_:) is called, unless you are using the main thread for both creating and sending. Which means the thread checking code is a little misleading as it implies you can use a thread other than the main thread, provided they match, but it's impossible to guarantee a match if you use any thread other than the main thread as far as I can tell.

I guess Store shouldn't be checking Threads specifically since TCA uses DispatchQueues as schedulers, as there is not a 1:1 mapping, other than for the main thread.

So far the only "workaround" I have found is to remove the thread checking code, which is obviously not a viable long term solution, as it requires a fork of the library.

[lukeredpath]

I've been giving this some thought but I've not thought of a good solution.

There is the dispatchPrecondition API which can be used to set a precondition that something is called on a specific queue however this will hit a hard breakpoint and doesn't give you much control over the error and it will assert in release builds which is not desirable.

However I am very much in favour of keeping this check for the most common use case - it also helped us find some spots where we had effects completing on a background queue.

Configuration options are always contentious as they increase API surface area what do you think about having some kind of boolean property that you can set to disable this behaviour on a specific store?

// This would be `true` by default but you can turn it off if you need to on either your root or a scoped store.
store.threadingChecksEnabled = false

[tgrapperon]

As I understand it, thread checks could work if restricted to the main thread.
A dedicated initializer may be used to confirm the intent that the store needs to run on the main thread. This may cover most of the cases, including the ones you had with isowords, and it would also bring back checks for main thread things in @mluisbrown case.
This may be an acceptable middle ground until TCA is converted to the new concurrency paradigm.
Edit: Alternatively, background stores can be opt-in via some initializer or argument as @lukeredpath suggested, and main thread ones, the default.
Edit: I've provided some sample implementation in #805.

[mluisbrown]

The implementation that @tgrapperon is proposing in #805 is what I was going to suggest, namely:

• make it explicit that the thread check is for the main thread (no need to compare with the initial thread).
• have an initialiser or factory method that allows opting out of the thread checking, for situations where that is needed.

[mluisbrown]

There is the dispatchPrecondition API which can be used to set a precondition that something is called on a specific queue however this will hit a hard breakpoint and doesn't give you much control over the error and it will assert in release builds which is not desirable.

There is also the setSpecific(key:value:) and getSpecific(key:) APIs which do allow you to reliably check that you are on a specific queue. Thanks to @gtsiap for finding this, and an example of it's use in the ReactiveSwift repo.

If we had a factory method for creating a Store that won't run on the main queue, as in @tgrapperon's #805, it would have to accept the DispatchQueue as a parameter in order to be able to set the key in Store.init.

[tgrapperon]

@mluisbrown I'm proposing an implementation using DispatchSpecificKey's in #807. This should bring back safety for any queue.

[mbrandonw]

Thanks everyone for all the great discussion.

We talked about it more this morning and we think there's a happy medium somewhere between what @tgrapperon has explored (which updates the API to opt-in/out of the behavior) and our current solution (which tries to automate the whole thing). PR coming soon.

[mbrandonw]

PR is here: #811

Let us know if you have any feedback.

[gohanlon]

What are your thoughts on @tgrapperon's idea to require that stores be created on the main queue unless there's an explicit expression of intent to do otherwise?

PR #811 would have thread checking disappear altogether when a store is created on a background queue, but it's fairly easy to mistakenly create a store off the main queue (right?). And, if the user is accustomed to generally receiving thread warnings when the store is used with improper concurrency, the lack of warnings as a side effect of their mistake could falsely imply that there aren't any concurrency mistakes.

[mbrandonw]

Hi @gohanlon, yeah that's a great point. We've pushed a change that brings it a bit closer to @tgrapperon's first PR #805. Essentially, stores created via the regular initializer will perform a main thread check immediately and stores created with the .unchecked static method will skip all main thread checks.

Hopefully that is a better balance in safety and hiding details from the users.

Check it out here: https://github.com/pointfreeco/swift-composable-architecture/pull/811/files

[gohanlon]

@mbrandonw Looks good to me!

(BTW: I think you meant to link to files of 811, not of #812 — busy morning!)