Update to thread warnings.

[mbrandonw]

A few small updates from #772

[stephencelis]

Nice!

[mamouneyya]

Now I have to do something like:

        return environment.databaseClient.importData(imports)
            .receive(on: environment.mainQueue)
            .fireAndForget()

just to avoid this warning. That doesn't make any sense to me, should this really include fireAndForget effects?

[stephencelis]

That doesn't make any sense to me, should this really include fireAndForget effects?

Unfortunately we think it must. As brought up in #772, when an effect's completion runs on the wrong thread, there is the possibility for there to be a runtime crash, so one must be diligent when working with background threads in general, even during fire-and-forget.

[mamouneyya]

Even when I am not mutating anything or doing any logic after (e.g. map)? If that's the case, how about TCA doing that for us always? It sounds like it's necessary no matter what..

[rcarver]

I find the new behavior surprising as well. The description of the original PR #772 said:

Any async Effect (including fireAndForget) could result in a runtime crash when updating internal state like effectCancellables

So at present would a fireAndForget effect only cause problems if it's also cancellable? Could we check if self?.effectCancellables[uuid] != nil before performing the thread check? (edit: hmm, can you safely check that without being on the correct thread?)

https://github.com/pointfreeco/swift-composable-architecture/pull/773/files#diff-50345ad35b59a32af74fda59695df9c6b1826302d5b41e5d8fbe47477917cc5dR393

More broadly, I feel like having to worry about scheduling back into the system breaks the semantics of 'fire and forget' (at least how I understood it). Related, if a fireAndForget is cancellable is it really fireAndForget?

[mbrandonw]

I know it sounds counter-intuitive, but even .fireAndForget effects must receive on the main queue. Such effects clearly don't emit anything, and so it seems unnecessary, but these effects call their receiveCompletion on the same thread as receiveValue, and that's why this becomes problematic.

If you look at the implementation of Store.send you will see that in the receiveCompletion block we do some work to clean up cancellables:

swift-composable-architecture/Sources/ComposableArchitecture/Store.swift

Lines 390 to 394 in 86bcd08

	receiveCompletion: { [weak self] _ in
	self?.threadCheck(status: .effectCompletion(action))
	didComplete = true
	self?.effectCancellables[uuid] = nil
	},

Note that this work has nothing to do with cancelling effects via the .cancellable(id:)/.cancel(id:) APIs. The cancellables in the above code snippet are there by virtue of the fact that we must subscribe to the effect returned to us by the reducer, and therefore we must keep around a cancellable.

So, even if you previously were not seeing crashes in your application's due to data races, the fact is that the possibility was always there. Effects completing on different threads will eventually cause a problem, even if they are "fire-and-forget", and this is why we'd like to warn loudly if we detect it.

Having said all of that, it does sound like we have room for improvement in the docs. Perhaps we should detail all of this information in the docs for .fireAndForget as well the Store/ViewStore docs. In the future we hope to leverage DocC to provide definitive sources for this kind of information.

[rcarver]

Right on, thanks @mbrandonw. Agreed this is totally a mental model and docs issue. I'd suggest including some info in the error message from this change as well. I'll update my app shortly and see if anything else comes up as I transition to this new understanding.

[mamouneyya]

I’m still curious if TCA can do this automatically behind the scenes, since it’s apparently needed for pretty much every case.

[mbrandonw]

Unfortunately there is no way to do that really. The simplest way to do that would be to chain on a .receive(on: DispatchQueue.main) to every effect returned to us from the reducer, but that can cause a lot of subtle problems. Alternatively, we could try introducing locks to protect the data in the store, but that will not be as efficient and can lead to all new problems.

As documented on the store we take a strong stance on not baking any threading/locking into the library at all. We feel the simpler approach is to make users of the effects be explicit with how they behave.

It's unfortunate that this change has caused disruptions, but we feel it's for good reason! In the future we'll try to do a better job of communicating this change to the community with better documentation, blog posts, etc.

[rcarver]

@mbrandonw quick note while I'm updating my app -

I added a receive(on:) to every fireAndForget and now I'm getting new failures from TestStore.assert - An effect returned for this action is still running. It must complete before the end of the test. which is great - I previously thought it was convenient but also odd that these effects go untracked.

Without diving into the internals of TestStore, I'm wondering if there's some more structure we could add to this whole thing such that the error could be "the effect [id]" instead of "an effect"

Would combining the need to receive and name each effect provide some clarity all around? For example:

return environment.service.execute()
  .receive(id: myId, on: environment.mainQueue)
  .fireAndForget() 

Similar to to how cancellation works now, but now for all effects. I wonder if this could simplify the mental model overall, since the rule would be to always identify and schedule any effects that receive/complete on a different thread.

[mbrandonw]

Thanks for the update @rcarver, it's good for us to know what the knock-on effects are of this change.

One thing that comes to mind with your situation is whether or not it is appropriate for you to use an immediate scheduler. Then you wouldn't need to worry about the completion. Are you using a test scheduler specifically because time is involved? If the only asynchrony in your effects is just simple .receive(on:)s then you could potentially simplify your tests with an immediate scheduler.

And as for identifying effects. It may not be clear at first, but the line the test failure points to is the action that kicked off the effect which is still running. So hopefully that should narrow down which effects could be causing this because you only have to look up that action in your reducer.

Adding an explicit identifier to the effect is interesting, and something we've actually explored a ton to help out with debugging. However, we don't feel we have yet solved the ergonomics around it. You end up having to thread the information through many layers, and it's quite easy to lose the information (basically every time you erase the effect). We want to spend more time thinking about it, but it's a hard problem.

[rcarver]

thanks @mbrandonw I'll try the immediate scheduler, that should simplify at least a few cases. I did know about the error line which is great - hopefully tracking down the rest of these failures will be easy enough.