Retained Task when run from long living effect

[rcarver]

Description

Ok I think I tracked down another leaking task — When a long living .run effect sends an action, and that child action performs async work, then the child task is retained until the parent task completes.

Checklist

• If possible, I've reproduced the issue using the main branch of this package.
• This issue hasn't been addressed in an existing GitHub issue or discussion.

Expected behavior

With this common pattern of observing a stream of values and relaying those to an action, I would expect any tasks spun up from those actions to be cleaned up while the parent task continues to run.

case parent:
  return .run { send in 
    for await value in await self.stream() {
      await send(.child(value))
    }
  }
case .child(let value):
  return .run { _ in ... }

Actual behavior

A Task is retained each time the child performs async work.

Steps to reproduce

1. Run the following code in Instruments with the Swift Concurrency tool.
2. Observe that the Task created by .child is in the Alive state until .parent completes

import ComposableArchitecture
import SwiftUI

struct LeakDemo: ReducerProtocol {
  struct State: Equatable {}
  enum Action {
    case parent, child
  }
  func reduce(into state: inout State, action: Action) -> Effect<Action, Never> {
    switch action {
    case .parent:
      // Because this task runs forever.
      return .run { send in
        try await Task.sleep(nanoseconds: NSEC_PER_SEC)
        await send(.child)
        try await Task.sleep(nanoseconds: NSEC_PER_SEC * 2)
      }
    case .child:
      // ❌ This task isn't released until the second sleep completes
      return .run { _ in
        try await Task.sleep(nanoseconds: NSEC_PER_SEC / 2)
      }
    }
  }
}

struct ContentView: View {
  let store = Store(
    initialState: LeakDemo.State(),
    reducer: LeakDemo()
  )
  var body: some View {
    Text("Leak Demo")
      .padding()
      .onAppear { ViewStore(store).send(.parent) }
  }
}

The Composable Architecture version information

0.42.0

Destination operating system

iOS 16.1

Xcode version information

Version 14.1 beta 3 (14B5033e)

Swift Compiler version information

swift-driver version: 1.62.15 Apple Swift version 5.7.1 (swiftlang-5.7.1.134.3 clang-1400.0.29.51)
Target: x86_64-apple-macosx12.0

[rcarver]

I tracked this down to these lines in Store.send, for both the .run and .publisher cases, where the parent task accumulates all sent child tasks:

if let task = self.send($0, originatingFrom: action) {
  tasks.wrappedValue.append(task)
}

Stepping away as any solution gets into subtleties of effect cancellation, and the heart of the issue may be a conflict with with cascading cancellation (testCascadingTaskCancellation)?

Here's a more real-world/interactive sample where you can see Alive Tasks accumulate forever every time the button is tapped. FYI it has some print statements because tacking on _printChanges() causes Store.send to run through the .publisher cases instead of the .run case.

import ComposableArchitecture
import SwiftUI

let relay = AsyncStream<Int>.streamWithContinuation()

struct SampleFeature: ReducerProtocol {
  struct State: Equatable {
    var value: Int = 0
  }
  enum Action: Equatable {
    case increment
    case receiveValue(Int)
    case task
  }
  func reduce(into state: inout State, action: Action) -> Effect<Action, Never> {
    switch action {
    case .increment:
      print(".increment")
      return .fireAndForget { [value = state.value + 1] in
        relay.continuation.yield(value)
      }
    case .receiveValue(let value):
      print(".receiveValue")
      state.value = value
      return .run { _ in
        print(" ** run ** ")
      }
    case .task:
      print(".task")
      return .run { send in
        for await value in relay.stream {
          await send(.receiveValue(value))
        }
      }
    }
  }
}

struct SampleContentView: View {
  let store = Store(
    initialState: SampleFeature.State(),
    reducer: SampleFeature()
  )
  var body: some View {
    WithViewStore(store) { viewStore in
      VStack {
        Button {
          viewStore.send(.increment)
        } label: {
          Text("Increment")
        }
        Text(viewStore.value.formatted())
      }
      .padding()
      .task {
        await viewStore.send(.task).finish()
      }
    }
  }
}

[mbrandonw]

Hi @rcarver, we were just chatting about this issue. We know roughly what the solution should look like and so should have a fix soon.

The crux of the problem is that we want an effect's lifetime to be extended to include the lifetimes of any effects created from sending actions inside the original effect. So, in your example above, if you navigate away from SampleContentView it will tear down not only the for await but also the effect returned from .receiveValue if it so happens to still be inflight.

It should be possible (hopefully!) to simply remove the children tasks from the array as they finish rather than waiting for the original effect to finish.

[rcarver]

Awesome thanks, that confirms my guesses and I look forward to seeing how to make the change 🙏🏻

[stephencelis]

@rcarver We had a few ideas but this was the least invasive one that seems to work:

https://github.com/pointfreeco/swift-composable-architecture/compare/weak-tasks

Not quite sure it's the right "fix" though. First, while it seems perfectly safe to cast a value type to AnyObject and back again, and it seems safe to weakify the AnyObject, I'm not sure we should be relying on the semantics of such a thing...if I can't dig up any related conversations on this kind of thing maybe I'll post a question on the Swift forums.

Second, should we even be bothering with this extra work? While the Concurrency instrument seems to consider tasks as "alive" even if they've completed and merely have a handle held somewhere, maybe this should be considered the "bug" and it should be reported to the Xcode team as feedback? Is there any harm in keeping these tasks handles around for the duration of the parent task?

[stephencelis]

I filed FB11690443 ("Concurrency instrument shows task as 'alive' even when finished") if anyone wants to dupe.

[stephencelis]

@rcarver Since it appears this isn't really a "leak" (the tasks are only retained as long as the parent is alive), and since my "fix" doesn't really do anything except work around a bug in the Concurrency instrument, do you think this can be closed? Or do you think there is a bug in our approach here that can be demonstrated/exploited?

[rcarver]

Hey @stephencelis thanks for taking a look. It's a great question, I wish I could offer more insight.

For context, my app has a bug where seemingly all async work stops happening. Like a queue is blocked, or priority inversion or something. I'm not sure, it's super random. I have yet to capture the bug while running in Instruments, so proactively I've been digging around trying to understand concurrency better. The Concurrency instrument shows me an ever-growing pile of Alive Tasks and that doesn't seem like a good thing. On the other hand, it could definitely be nothing. Any idea of the cost of holding onto a Task handle?

The app basically has a render loop, so any change to state results in an accumulated task that lasts for the lifetime of the app.

Pulling this branch now and I'll see if it helps. Any ideas to solve my actual problem? I'd happily drop this issue :)

[stephencelis]

@rcarver Let us know if it does!

Task handles should be super lightweight, though, just 8 bytes per task in this array, so I imagine you'd have to be accumulating a ton of tasks for this to be noticeable.

As for fixing...hard to do more than speculate.

A non-library-level solution would be to restructure your render loop in some way so that not all effects come from a single starter effect. I think this would mean using a view store to coordinate the run loop instead of the reducer's effect layer, if that's possible.

Library-level fixes may require inserting/removing tasks a bit more dynamically in send. Tasks are hashable, so we could maybe use a set instead of an array there, but there's a bit of a chicken-egg problem where we want tasks to do cleanup work when they're finished. A few potential thoughts if you want to explore a fix:

• We could fire off an additional task for every task appended to the set. It would simply be responsible for awaiting the task's value, and then removing that task from the set. This would mean double the number of tasks spun up, but the cleanup tasks would be able to remove the other tasks more quickly.
• Maybe the tasks could use withUnsafeCurrentTask to look themselves up and remove themselves from the set of tasks.

We'll think on it more, but any insight you can give us, or project that can reproduce a real problem, would be helpful. Short of that it's hard for us to really say this is a bug with our implementation or not, and so not sure we should track it as one in the long run.

[rcarver]

Sounds good @stephencelis.

Instruments shows zero accumulation of Alive Tasks with your branch. I'll keep an eye out for the actual bug on this build. Either way hopefully this will lead to some insights.

[stephencelis]

For what it's worth I think it'll still accumulate "weak tasks" 😉

[stephencelis]

@rcarver I'm going to move this to a discussion for now. While there are potential improvements we could maybe make, it's hard to measure the problem right now. Please do follow up in the discussion with your findings, and if you find that are workaround somehow prevents the issue, we're definitely down to merge that branch or something like it in the future.

And if you are able to more generally help us reproduce the problem that ties it to our current implementation of send, we can start a new issue then.

[rcarver]

Quick update:

1. My "app stops responding" issue persists with your branch, so it's not the answer. I'll be on this this til it's fixed, will report back!
2. Despite the trickery just to "fix" Instruments, it was nice to confirm that the rest of my app isn't hanging onto tasks.