This review is part of the large concurrency feature, which we are reviewing in several parts. While we've tried to make it independent of other concurrency proposals that have not yet been reviewed, it may have some dependencies that we've failed to eliminate. Please do your best to review it on its own merits, while still understanding its relationship to the larger feature. You may also want to raise interactions with previous already-accepted proposals – that might lead to opening up a separate thread of further discussion to keep the review focused.
Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager. If you do email me directly, please put "SE-0302" somewhere in the subject line.
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:
It's fine to take inspiration from other runtimes, but you have to take into account the complete concurrency story of Swift here. I.e. things which other "just async await / coroutines" do not offer: actors.
You propose:
Which is not something that we'd want to do.
Rather, the same is expressed by an actor being on a specific executor:
actor Someone {
let serialExecutor = ... // some specific queue or executor here
func doFirstThing() throws -> String { ... } // will execute on the actor's executor.
}
Konrad notes that this is similar to async let, which we separated out into a different proposal because it is syntactic sugar on top of task groups. However, there is an important philosophical choice here: the last paragraph of the section that introduces child tasks and task groups says this:
By contrast with future-based task APIs, there is no way in which a reference to the child task can escape the scope in which the child task is created. This ensures that the structure of structured concurrency is maintained. It both makes it easier to reason about the concurrent tasks that are executing within a given scope, and also unlocks numerous optimization opportunities for the compiler and runtime.
I'm not quite sure from your comment whether veggies, meat, and oven are supposed to be of type Future<T> or T. The async let syntax ensures they have type T, and the future is intentionally hidden. The paragraph above highlights that we explicitly don't want the shorthand syntax for creating child tasks to create Future<T> instances, because it makes it too easy to violate the structure of structured concurrency.
I'm curious about the asymmetry of the naming of Task.runDetached and Task.withGroup.
My understanding may be incomplete, but it seems like they each create and run a task, just in different manners - one detached, one with a group of child tasks.
I am very +1 on this proposal, only have some nitpicks on writing and some minor suggestions and questions below. The proposal is very well written, introduces many new concepts and explains them clearly. Several of these concepts (incl executor etc) are common with actors, but including them in this proposal works well.
Yes, yes.
Yes, I've worked with numerous other threading libraries and built relevant systems etc, but I've never worked with a system that has structured concurrency like this. This seems like a great progression vs state of the art and builds on many strengths of Swift.
I've spent a lot of time on this. I've participated in the pitch threads, and did an in-depth study of this proposal.
Here are thoughts and comments on a detailed read-through of this proposal:
Task is a great name for this concept, I'm glad we're staying away from otherwise overloaded terms, like thread.
I'm very happy with much of the framing and very clear delineation of the goals of various subsystems, e.g. cancelation.
A writing nit: "Detached tasks" are first mentioned in "task priorities", it would be nice to explain it a bit earlier in the conceptual section.
Task groups are a really nice abstraction, it will be even nicer with some APIs like parallelMap and "fork" that takes away the boilerplate.
Does the example in Task groups and child tasks work? The closures passed to group.add cannot capture veggies/mean/oven by reference, I think these are more naturally returned up.
In the section about unsafeCurrent I'm very happy to see that sync functions can get a handle to their current task, but it is weird that they are forced to deal with unsafety to do so, at least in the case where they just want a Task and don't want to do fancy things.
The call to Task.unsafeCurrent is not itself unsafe, it is the returned UnsafeConcurrentTask that is and the possibility of it escaping. For sync clients that don't need the unsafe stuff, I think we should something like:
extension Task {
static var unsafeCurrent: UnsafeCurrentTask? { get }
static var current: Task? { unsafeCurrent?.task } // safe for sync callers!
}
That seems perfectly safe and is the thing we want to encourage. Given that, does current() need to be async? Also, shouldn't current() be a property now that getters can be async? I imagine that the async version may be more efficient (?) and if so it seems these should be overloadable given the rules in the async/await proposal.
The separation between UnsafeCurrentTask and CurrentTask is great.
It's great to see the priority API built out. Writing nit that these two paragraphs are a bit redundant:
The "Task.Handle" API looks nice. It is great that it supports a typed error generic argument.
let dinnerHandle: Task.Handle<Meal, Error> = ...
try await eat(dinnerHandle)
should be try await eat(dinnerHandle.get()).
In Task Groups, it looks like withGroup always throws. Wouldn't it make sense to support a body that doesn't throw, either by overloading this or by using rethrows? Is the type system not powerful enough (because it would have to flow down to Group.add) to model what we need here?
Should Task.Group have a generic error argument to mirror Task.Handle and more naturally allow for typed errors in the future? The existing withGroup could always force Error, but this would permit other withGroup-like members in the future.
I still think that async let is better done as a spawn/fork library feature, along with parallelMap, etc, but that can be a discussion for another day of course.
Overall, really great work. I'm very excited to see this come together!
This difference is on purpose and it reflects that a withGroup does not create a task by itself, it very much runs "in" the same task that it was called from. Notice that the withGroup's body is neither @escaping or @concurrent:
func withGroup<TaskResult, BodyResult>(
// ...
body: (inout Task.Group<TaskResult>) async throws -> BodyResult // not child task
) // ...
// as compared to:
static func runDetached<T>(
// ...
operation: @concurrent @escaping () async -> T // operation in new task
) // ...
It only creates a scope in which tasks are created and run: the ones added to it with await group.add { <i'm a child task/> }.
Thanks for the review comments Chris, diving in then:
Yes, that's correct. @John_McCall strongly suggested we call the function to reach for it unsafe though as often times you may not even see the returned type, e.g. if you'd close over it from a different task and touch it from another task etc... It felt important for the name to imply users have to be careful here.
That would be nice, but it'll cause us to overload a property with a different return type depending if it's async or not... I'm not completely up to date what our plan with overloads is, it kept changing back and forth, but today we can't write this:
Task.swift:541:21: error: invalid redeclaration of 'current'
public static var current: Task? {
^
Task.swift:52:21: note: 'current' previously declared here
public static var current: Task { get async {
^
We also can't add the current computed synchronous variable, even if we kept the async function:
Task.swift:540:21: error: invalid redeclaration of 'current'
public static var current: Task? {
^
Task.swift:52:22: note: 'current()' previously declared here
public static func current() async -> Task {
^
So yeah, interesting idea but today we can't write the synchronous current because of redeclaration issues. @Douglas_Gregor would know what our end goal plan with those is.
Yes, and the proposal itself has a comment about this:
static func current() async -> Task { get }
// If async computed properties proposal is accepted, this can become a static property
It is one of those proposal inter-dependence things... The async properties are still in pitch phase [Pitch] Effectful Read-only Properties but yes, the intent is for current to be an async getter.
I think it's best to keep the proposal in sync with the implementation shape for now, until the async properties land at least, to not confuse people about what API is "real".
In this specific case it does not matter for efficiency if the func is async or sync -- both end up in the same actual implementation (which eventually hits a thread local or other platform specific storage for it).
Yeah we're missing a way to express the overload, Doug should know our plans with overloading, I'm not sure about those.
That's actually correct, it shows you can pass around a handle (for better or worse), because the eat is declared as:
func eat(mealHandle: Task.Handle<Meal, Error>) {
I will have added the missing mealHandle: label, that'll explain things better.
Oh that's a great catch, seems we are able to express this - thanks! A rethrows does the job here, given the current shape of the groups APIs, hooray for non-escaping body.
I'll adjust that right away.
I guess we might have to if other APIs under review are doing the same now - i.e. the UnsafeContinuation APIs moved towards a typed error even through it only ever is Error.
I guess it'll be an extra childTaskThrowing: Failure = Error.self parameter
WDYT @Joe_Groff ?
Thanks for the review, plenty useful feedback here
await Task.withGroup(aggregating: Int.self) { group in
await group.add { 1 }
}
I'd welcome nice spelling ideas for all those parameters.
Should group.next() assert that it is called correctly?
It is by design and important for performance and correctness of a group that next() be only invoked from the task running the group.
We could assert(Task.unsafeCurrent == _task) check in next() if we really wanted to, but it feels like quite a performance hit to be hitting current task for every single next() call so I'm on the fence about it. It's also why I was considering (and currently doing so) only checking this in debug mode.
The next() function is on purpose declared mutating in order to make it hard to use it incorrectly by escaping it etc. The group is NOT Sendable.
Any opinions on this one?
Suspending group.add
Food for thought - today we always await group.add but we never actually suspend there.
We want to be "future proof" that at some point we'll have groups that control the number of added tasks... but we have not designed this at all. I am wondering if this is future proof, or troublesome.
I do feel like we may need various types of "add" eventually. Good names I had in mind were things like launch, spawn etc. But we'd need to align those with whatever async lets end up doing etc.
Just bringing it up for reviewers to consider.
Following up on this one:
Yeah so that's true, and I'll adjust that, however in practice it does not win us much, because we're unable to correlate (not way to express in type-system), the following relation:
// (NOT REAL API)
await group.add { 1 }
// "aha, we never throw in any of the child tasks!"
// thus...
let one = /*try*/ await next() // we don't have to throw from next
So next() always is throwing, because we don't have a way to relate it to the fact that no child tasks would have had the ability to throw. So realistically, the vast majority of group implementations will be throwing, because they have to throw out of the next().
People couldtry? await next() or try! await next(), so yes the rethrows on the withGroup does matter, but sadly won't completely solve the sometimes throwing even if we know we won't issue.
The same is true with consuming the group as an async sequence:
for try await r in group { // forced to try since next() throwing
}
We could solve it the same way as we do with UnsafeContinuation, by duplicating the types:
await Task.withThrowingGroup(accumulating: Int.self) { group in ...
var sum = 0
for n in 1...10 { await group.add { return n } }
for await r in group { sum += r } // no try!
return sum
}
try await Task.withThrowingGroup(accumulating: Int.self) { group in ...
var sum = 0
for n in 1...10 { await group.add { throw Boom() } }
for try await r in group { sum += r }
return sum
}
do we want to do this though?
(Just adding a Failure parameter does not solve it, because we have no way to "hide" the add(body: throws async) function from groups which are Failure = Never even if we can hide the not throwing next()).
I feel like an idiot for writing this post because what follows will not substantially address the fantastically complex work done. Concurrency programming isn't my strength and I'm super excited for Swift 6. But I want the user experience of an API to be as elegant as possible, guiding me to do the right thing when I may not be an expert with just the right amount of complexity when I need it.
When reading the proposed solution, I get the impression Swift developers may likely have to deal with this API for anything more complex than doing something with the result of a series of fire and forget async let s. To my mind, this could obtain often (e.g.: cancelling some long running work for the user, like a network request, processing some data in a database etc).
Now, perhaps I'm an overly spoilt Apple/Swift developer and perhaps I'm expecting too much abstraction from a proposal that could be aimed at defining the lowest level, but the Task API seems a bit clunky or foreign in an industry strongly disposed towards declarative approaches to solving problems. The API also strikes me as conceptually intimidating to someone who suddenly realises they need this kind of complexity. I am also concerned about how teachable it is, either self directed learning or teaching others. Are these observations only occurring for me?
A completely naive, ideal state, at least for me, could look something almost like a declarative version of the meal cooking example from the proposal. Taking some inspiration from result builders, I feel like (all the feels and opinion), this is easier to understand (you just build the Task result by composing the tasks to be done, no direct use of handles, explicit cancellation, arguably, could be simply setting a boolean or calling a function), much clearer to read (it's clear what is grouped), less boilerplate (no need to specify return types when calling functions, etc):
I'm sure this example does not capture the full range of functionality proposed. Might not be consistent, either. But it should convey the ideas. But what do people think? Am I the one having these feelings about the user experience of this API? Or am I expecting too high level of an abstraction in a proposal defining the lowest level? Anything else?
Hi Kiel,
Thanks for the feedback - I understand the concerns you're expressing (I think at least)
The specific difference where async let (not part of this proposal, but coming soon, so very much right to allude to it when comparing the two) is "not enough" is dynamic numbers of tasks. Note that the dinner example does not have dynamic numbers of anything, it is compile time known how many tasks there are, and you don't really do anything dynamic with them, you await on them one by one in a compile time defined order and that's about it.
It may be a bit misleading to only discuss groups with relation to the dinner example. We use the makeDinner example to showcase how async let desugars into a task group, but it's not the only or main use case.
Consider the following problem: "spawn 10 child tasks, return whichever 2 tasks complete first, cancel the remaining ones". It's not really possible to express this without some form of group-like entity. As it is impossible to express the "whichever 2 tasks complete first" by just writing await in some specific order, it has to be some form of "group" or "select" we put them into, and at runtime we'll get the right behavior.
Now, how this is surfaced as end-user API is an entirely different discussion, such functionality would obviously want a nicer API than everyone having to reimplement a manually doing that task group by hand... Thankfully, we can build it in the library by using a group:
// probably contains silly mistakes, just to prove the idea
func gather(first expected: Int,
items: [Item],
makeWork: (Item) async throws -> Work) async throws -> [Work] {
try await Task.withGroup(resultType: Work.self) { group in
precondition(items.count >= expected)
for i in items { await group.add { await makeWork(i) }
var result: [Work] = []
while result < expected {
let r = try await group.next()
result.append(r)
}
return result
}
}
usage:
let two = gather(first: 2, items: [...]) { await makeWork($0) }
assert(two.count <= 2)
Similarily with some "parallel map" or other typical idioms, they all should be implementable in terms of a group.
I also have hopes for various result builder powered mini DSLs which relate to automatic progress monitoring of tasks and similar powerful patterns... they all end up being implemented in terms of groups, but you never see the groups in the high-level APIs. Especially if we did the future direction of result buidlers I had requested a while ago for different reasons: https://github.com/apple/swift-evolution/blob/main/proposals/0289-result-builders.md#transforming-declarations and allow them to handle async let, I think there is tons of potential to express nice idioms using such mini DSLs (like gather, select, firstOf, retry etc...) using plain library code, yet they all would be really operating on an underlying task group.
It is important to have powerful low-level building blocks, in order to build on top of them. The "power tools" are going to be a bit harder to use than the things we can build on top of them, but it's important that they're efficient and flexible.
Libraries can sugar over the core primitives as showcased above, and async let and other sugar should be able to address the usability concerns fo the majority of use cases. So I think you got it somewhat right with:
So yeah, it feels like you're asking for high level syntax sugar for things -- this proposal isn't bringing this to the table, but the follow-up of async let will be.
Hopefully async let and friends will be just that though
Seems like reasonable future-proofing, though we don't currently have defaultable generic arguments today, so this would need to be a separate form of withGroup. Perhaps we could do what we did with the continuation API and have withGroup and withThrowingGroup for the common Never and Error cases.
Ok makes sense. Sorting these two out seem important since they're the same thing, just one is async and the other sync. I hope this will settle out cleanly as the other proposal comes in.
If that is the case then why keep the async one? Just drop it and only have the sync one and you should sidestep the whole overload issue.
Cool thx.
I think this is a very good point. I would recommend changing this to be sync, and then have a "addWithBackpressure" future overload that would be async for the clients who care. It doesn't seem like something that everyone should be forced to eat the complexity of when most people won't care (progressive disclosure of complexity and all).
Also, per upthread comments, I think that "TaskGroup.add" should have the word "run" in it for consistency. "withGroup" creates a great, but "add" actually creates a runnable thing and reflecting that in the name seems like a good thing. Alternatively, your suggestions of launch, spawn, etc could all be chosen instead of run with different tradeoffs.
This is what I meant by "not being able to express it in the type system. I think the solution here is to have two different Task.Group types, and have a "Task.withGroup" and "Task.withThrowingGroup" (better names welcome ;-) that return a Task.Group and Task.ThrowingGroup type respectively. This allows the add and next methods to have the right result signature for the different use case. You might be able to overload the two Task.withGroup's based on the throw-ability of its body?
Oh I see you suggest this in "duplicating types"; exactly that.
This is pretty important because effects like 'throws' pollute the entire call stack. I think this is probably worth it. typed throws with support for "throws Never" in generic situations would make this all pretty and generic instead of copied, but we don't have that yet.
Is there an overly clever thing that you could do by making "group.next" be inlininable and contain 1) the assert and 2) a call to an uninlined nextImpl? This would allow the client to disable the assert based on its idea of whether it is debugging or not.
The right answer here is to get ownership types into Swift of course.
The async one is the only way to get a task without touching an API that has "unsafe" in the name (or some form of "unsafe" in the name), so I feel we need it.
To reiterate, the APIs to get a Task are only those:
let task: Task = await Task.current // once async properties
let unsafe: UnsafeCurrentTask? = Task.unsafeCurrent
let task: Task? = Task.unsafeCurrent?.task
So we can't easily get a task without doing quite a lot of ceremony in an async context if we removed the async property: 1) we'd have to dance with the optional 2) we'd have to use an unsafe API to obtain it.
It might be true that people rarely should need an actual Task because all useful functions we most functionality is available statically (Task.isCancelled etc), but still, it's useful enough not to make it super ugly and through unsafe API hm. A weird but important example is getting a Task once before a hot loop, and only task.isCancelled checking it inside the loop -- this avoids continious thread local lookups (typical pattern recommended in intel guidelines etc...).
Throwing / not throwing Group
Right yeah, it's some duplication. I also poked around with some sneaky tricks how perhaps we'll be able to not duplicate the types... (though it's a bit nasty, the add would also only be conditionally added to the type, depending on Failure == Never and Failure == Error).
I'll PoC this a bit more but I think we agree that we need to support this non-throwing group nicer. I'll get back to the review with the specific shape how to achieve this soon.
No idea if we can pull such trick... Maybe @Joe_Groff will know.
Yeah for consistency it seems like it might want to have run in the name.
I'm a bit concerned about making "the one we want people to use", i.e. the backpressuring one (if we indeed decide to keep it) have a long name. My philosophy with names has always been to give the shortest name to the API I want people to use, so here this would be await group.run { ... } or await group.attach { ... } or await group.add { ... } etc, and an "add unconditionally" one would have a longer more annoying name, like group.addForcefully { ... } or group.run(ignoringCancellation: true) { ... } or something like that. Fishing for ideas here, and I'm not sure what we'll do with the suspending version -- we should decide if we'll indeed be using it or not
I'm also tempted to talk about launch and detach but I guess they're just "cute names" I like, and not necessarily adding to the clarity, so I guess the more explicit names may be fine.
