Warn if send is called after .run completes

[kabiroberai]

Calling send after .run returns can result in broken assumptions and discarded actions.

• If the bad .run is directly sent into a Store, the send calls will work but they won't be tracked by [ViewStoreTask|TestStoreTask|TestStore].finish()
• If the EffectTask.run is converted into a Publisher by applying a Combine operator, the resulting Effect will entirely ignore actions sent after the async closure returns, since EffectTask.publisher marks the effect as completed at that point.

This PR adds a runtime warning that's emitted if send is called after the .run closure returns, formally marking this as undefined behavior.

.run { send in
  Task {
    try await Task.sleep(for: 1)
    await send(.action) // now emits a warning
  }
}

[pyrtsa]

Do we really want to mark it as @MainActor?

[kabiroberai]

We need the reads and writes to isCompleted to be serialized, and since the Send closure is @MainActor it's sufficient to make this one @MainActor too for that purpose. This doesn't mean that operation will run on the MainActor, but rather just the synchronous parts of the task (i.e. the isCompleted = true). We're already doing a MainActor hop in the Publisher case so this isn't straying from established practice.

[pyrtsa]

For clarity, so the problem is in user code not taking part in structured concurrency of .run { send in ... } but instead wrapping their logic in a non-structured Task { ... }. Nice idea to warn about that!

[kabiroberai]

thanks, yeah I wanted to mention "structured concurrency" in the runtime warning but figured it's better to avoid jargon. FWIW there are a number of other ways to trigger this such as calling a completion/GCD-based API (eg DispatchQueue.main.asyncAfter) so "switch from unstructured to structured concurrency" isn't a blanket solution. Users may need to use other synchronization primitives (continuations, AsyncSequence, task groups) instead.

[mbrandonw]

Hi @kabiroberai, thanks for the PR! Overall I think this looks good! I took a pass at playing with the messaging, but Stephen and I will discuss more. I also split the test to focus just on the publisher API versus the pure async/await API.

We should be able to merge this soon.

[kabiroberai]

@mbrandonw btw I think CI is failing because the check is guarded by #if DEBUG, so the release tests don't work. We can conditionally compile the tests too, or maybe make the check run in release builds too — up to you folks.

[mbrandonw]

🤦‍♂️ thanks for pointing that out. Was confusing me. Yeah I think it's OK to do this check in release. We already have other runtime warnings in release. Want to fix that up if you have the time?

[kabiroberai]

@mbrandonw so it looks like the runtimeWarn impl is guarded by #if DEBUG anyway; think we'll have to make the tests debug only to make them pass

[mbrandonw]

@mbrandonw so it looks like the runtimeWarn impl is guarded by #if DEBUG anyway; think we'll have to make the tests debug only to make them pass

Sorry again, I should have looked into this more closely. You are right, runtimeWarn only causes test failures in DEBUG because we can't ship the dynamic XCTFail stuff in release code due to App Store rejections.

[mbrandonw]

Finally got tests to pass 😅. Custom executors can't come soon enough.

@stephencelis and I will chat about this today and hopefully get it merged!

[stephencelis]

Looks good to me! Thanks for tackling this!