Structured concurrency: Updates from review #1

[ktoso]

Updates form first review

[ktoso]

I discuss the removal of await here a bit below.

The short version is:

• we never implemented this "suspend as a form of back-pressure" nor are we even confident it is the right thing to do, rather than some more holistic take on the concept.
• that await was tremendously confusing to reviewers -- "it is suspending until the operation completed?" was very frequently asked.

[ktoso]

this had to use the enum dance due to the strong @Concurrent closure checking on main now

[ktoso]

note that implementation wise this is a bit of hell --- we have to duplicate the TaskGroup struct impl twice, once normal and once Throwing but there is no way around it.

It is the "if any closure submitted to spawn throws, then next() must be throwing" so we can't just rely on rethrows.
It is even more complex with the AsyncSequence conformance, so sadly we have to duplicate implementation a bit here.

[ktoso]

I would also suggest doing:

group.spawn -> Task.Spawned<T, Failure>

  public struct Spawned {
    /// Returns `true` if the task was successfully spawned in the task group,
    /// `false` otherwise which means that the group was already cancelled and
    /// refused to accept spawn a new child task.
    public var successfully: Bool { handle != nil }

    /// Task handle for the spawned task group child task,
    /// or `nil` if it was not spawned successfully.
    public let handle: Task.Handle<ChildTaskResult, Never>?

    init(handle: Task.Handle<ChildTaskResult, Never>?) {
      self.handle = handle
    }
  }

since then it is just a single API to handle both:

• did we spawn successfully or was the group cancelled already
• cancel this specific task
• wait for this specific task

[ktoso]

Another alternative design for this is with throwing CancellationError however then ONLY the withThrowingGroup could do so, and we'd need some new name for trySpawn (try spawn is terrible since it becomes try group.trySpawn { })

withTaskGroup(of: Int.self) { group in 
  // always spawns:
  group.spawn {}
}

withThrowingTaskGroup(of: Int.self) { group in 
  // always spawns:
  group.spawn {} 

  // spawns, unless the group/owning-task were cancelled, then it throws:
  try group.spawnUnlessCancelled {} 
}

So that'd be another option; and the spawn would be Void returning OR returning Task.Handle and Task.Handle? in the "may fail" case.

FYI @airspeedswift for when you have the time to review; plenty of various tradeoffs here, I hope I explained them well though, it is mostly:

• ability to not spawn in a cancelled group
• ability to always spawn (as async lets would do, if we express them as "implicitly in a group" which I think we should, but I did not manage to do this with my beginner SIL skill...)
• ability to get a handle for a specific spawned task

The Spawned addressess all in one API call though one has to search for it. The throwing versions are very annoying, and can only be available in withThrowingTaskGroup. So there's some tradeoffs to make here I'd need input on. Thanks in advance!

[jckarter]

You can still spawnUnlessCancelled inside a non-throwing group, you'd just need to handle the CancellationError locally and not let it propagate out of the group block:

withTaskGroup(of: Int.self) { group in 
  do {
    try group.spawnUnlessCancelled {}
  } catch { print("couldn't spawn new task") }
}

[jckarter]

If the error handling ceremony is too onerous, another option is to have spawnUnlessCancelled return a Bool like add was previously proposed to, but remove the @discardableResult from the declaration, so that you have to look at the result. I don't think it needs to return more information than a Bool, though. That way you can use spawn in the "don't care" case without worrying about implicit dropping of work you don't expect, and you can use spawnUnlessCancelled when you do expect to stop adding work in the face of cancellation.

[ktoso]

Great points, thank you! I'll adjust this right away so it is part of the review 👍

[ktoso]

(This of course is just var sucessfully: Bool { handle != nil } so does not need storage)

[jckarter]

I don't think that having spawn return a more elaborate return value is much better than returning Bool—you still have the problem of wanting to ignore the return value most of the time, and it then being easy to forget to pay attention to it when you need. Being able to get a Handle to child tasks also strikes me as an anti-feature, since it creates the "two classes of Handle" problem where some handles implicitly blow up and start trapping if you access them out of scope, and it also enables unstructured use of the handle within the group.

[ktoso]

I don't think that having spawn return a more elaborate return value is much better than returning Bool—you still have the problem of wanting to ignore the return value most of the time, and it then being easy to forget to pay attention to it when you need.

Yeah I think you're right here... sounds like a not ignorable bool return sounds great here, thanks!

Being able to get a Handle to child tasks also strikes me as an anti-feature, since it creates the "two classes of Handle" problem where some handles implicitly blow up and start trapping if you access them out of scope, and it also enables unstructured use of the handle within the group.

That's a great point, thanks that convienced me that indeed is an anti feature.