SE-0304 (2nd review): Structured Concurrency

Evolution Proposal Reviews
22

Unlike the current function, the withUnsafeCurrentTask function returns an optional UnsafeCurrentTask ...

This may be a nitpick, but withUnsafeCurrentTask doesn't return the task; it provides it to the callback.

23

Ah, that makes sense. Thank you for the clarification.

In this case, I would prefer

run {
    // work
} onCancel: {
    // Cancellation handler
}

or

execute {
    // work
} onCancel: {
    // Cancellation handler
}

Naming the function withTask... would go against the convention that a function having the prefix with provides an argument to its closure argument (such as withTaskGroup(), withUnsafeCurrentTask () or withUnsafePointer()). In the case of withTask(), one could assume that the closure has an argument of type Task.

4 Likes
24

This iteration is greatly improved, but needs some further refinements. I will get into more details if I get a chance. (Too busy as usual)

I will only address this for now:

I think it is conceptually close to a do {} catch {} block. Maybe we should use similar order and naming. I don't think using words other than do would be a good idea. Words like run and execute create other expectations that do not hold. Since the handler runs concurrently, and needs a colon, we may use something other than catch for it. I think cancellationHandler is already fine.

How about:

doCancelable {
    print("Work")
} cancellationHandler: {
    print("Cancelled")
}
4 Likes
25

This is stellar :star2: I love the new naming :heart:

Is it on purpose that the Task.current computed property is referred to as a "function"?

26

The proposal got a few functions that are declared static in the definition, but are used without Task. in the examples, including withUnsafeCurrentTask, detach, withTaskCancellationHandler, withTaskGroup, withThrowingTaskGroup.

I'm now quite confused which functions are moved to top-level, and which aren't.

1 Like
27

It should be clear by the way that non top-level ones are always declared in an extension, but yes there remained a few stray static keywords. Thanks @nikitamounier for spotting those and that's now fixed https://github.com/apple/swift-evolution/pull/1322

Also fixed that nitpick along with it

1 Like
28

The doCancellable makes it easier to discover (or be reminded of) via code completion of just do. Though cancellationHandler Is rather verbose and less action-y. Perhaps pair it with onCancel or onCancelled?
Though again, having both keywords end on something with cancel is less ideal in my view.

I assume overloading do is not possible? do {} onCancel: {}

1 Like
29

Unlike the current function, the withUnsafeCurrentTask function returns an optional UnsafeCurrentTask , this is because such synchronous function may be invoked from a task (i.e. from within asynchronous Swift code) or outside of it (e.g. some Task unaware API, like a raw pthread thread calling into Swift code).

This feels misleading to me for two reasons:

  1. The word "optional" is highlighted which I don't think it should. I think the key part here is "Unsafe".
  2. The text explains the optionality, while it's really the unsafetyness that's interesting. The optionality seems the same as for the current property described in the section above this one.

If we currently do not have anything to put in the UnsafeCurrentTask, would it make sense to postpone that API to a future proposal that needs it?

30

It is not possible with a library function. Moreover the proposed function returns a result, while do is a statement. They look similar, but are very different. Also, I think we need an overload without the actual cancellation handler for the times when we just need to make an operation interruptible without any special cleanup.

About onCancel: I am a bit hesitant about it. on- prefix is used in many frameworks, but I don't think it is well suited for standard library use.

1 Like
31

I was going to write essentially the same. UnsafeCurrentTask seems to be only related to task local values at the moment.

In what Task.current and withUnsafeCurrentTask should differ? Task.current can be called in both sync and async contexts, in functions running as part of a task and in functions that are not part of any task, and its implementation is exactly the same of withUnsafeCurrentTask (they both call _getCurrentAsyncTask() internally).

32

The separate type and API is necessary to expose write operations, such as writing a task local value which are a separate proposal that is waiting for this one to conclude.

This proposal does not expose any write operations to tasks at all, so yeah they're equal-ish. That does not mean there is no need for the UnsafeCurrentTask though. Removing it is not viable, we'd only be pretending it's not necessary while we know it is.

2 Likes
33

I still think it'd be better to defer its introduction until more is exposed about UnsafeCurrentTask. I don't see how it'd be a problem to hide this part of the API until later. Many features are also prefixed with _ until its shape is fully fleshed out.

It's not exactly helping to know that it'll be used somehow. I mean, I'm sure you have a clear picture of what operations this UnsafeCurrentTask can support, but that is not laid out, making them unchallengeable. That doesn't make for great proposal material.

5 Likes
34

Another small nitpick re. "withTaskCancellationHandler()":

Note that the handler runs @Sendable with the rest of the task, because it is executed immediately when the task is cancelled, which can happen at any point.

This used to be "@concurrent". Perhaps "@Sendable" should be replaced with "concurrently" now?

2 Likes
35

Please feel free to send PRs to fix minor typos like that; they don't really need much discussion.

36

I'm 50/50 on this, just because I work in a few legacy codebases that have tons of top level functions and it can be hard to discern what's provided by the language/1st party frameworks and what's something we've custom rolled - that makes me want to lean more towards withTaskCancellationHandler.

However, in newer code, and as time goes by with more adoption of Swift's native concurrency model, I'd think I prefer withCancellationHandler

I agree with others that having it operation - cancel handler in both because it matches the same mental model of do ... catch, but when using both multiple-closure syntax and explicit param syntax, it explicitly calls out the block that is actually doing the cancellation, which is the most important part IMO of that function.

In general I'm very happy with this pitch, but if some of it is still open to litigation, I'm not sure the group.spawn name is correct or well-named in practice.

I didn't follow the first review thread at all, and when reading through this new draft and first coming across it the combination of the closure returning a value plus the name really threw me off.

The previous group.add made much more sense in the structure of... well structured concurrency that you're adding a new subtask result to the overall group result.

The free-floating function detach makes 100% sense to me, though.

2 Likes
37

Apologies for a comment that is not directly related to the feedback requests ... I just wanted to voice one more question related to a question from the prior review thread ...

In the prior review I had commented wishfully about wanting an effect in the type system to prevent async method implementations from being able to create detached tasks. Chris Lattner responded thoughtfully that this kind of type system effect would probably not fit well into the swift language in a composable way ...

I had meant to ask another follow up but failed to do so ...

I can understand why an effect to provide static prevention of new detached tasks might be a bad idea -- but what about an api to dynamically prevent creation of detached tasks within an async context? I'm wondering if there should there be a way to assert that once code execution has reached a certain point in an async context that any subsequent attempt to create a detached task from within that async context will fail?

An api like trio's I believe would allow this by creating some kind of wrapper around the nursery and passing that along rather than the fully capable nursery object -- but I'm not certain if you could arrange for code that obeys this kind of constraint in the swift api's?

I'm not entirely sure why I think this is important ... some intuition i have that there will be common code problems that will lead to a lazy developer tendency to overuse detached tasks rather than finding more appropriate solutions that fit into structured concurrency ... and I feel I will
come to know a general fear of hidden uses of detached tasks from things like - say - a third party api client library ...

Maybe I'm wrong but Im worried that in practice the detached task hole out of structured concurrency will leak like a sieve out of which many of the simplified reasoning goals (exactly once success or failure over a composition of async interactions) offered by structured concurrency will be lost in practice ...

In the back of my mind when I
think about structured concurrency I think about how applications like mail.app used to offer views to show and control what was going on in the application while talking to a remote mail server and even allowed for particular "tasks" to be cancelled by the user ... it's my hope that structured concurrency could lead to the return of applications which provide this kind of visibility and control over what the heck an application is doing while it tries to do stuff over a network ... -- but if task implementations are all tending to launch detached tasks that effectively lose their relationship to whatever high level application concept lead to their creation I feel this hope for better visibility into application behavior will probably not be widely realizable in practice ...

3 Likes
38

I'm not too worried about people overusing detached tasks, because soon we'll have a similarly intuitive construct for creating child tasks (hopefully async let, which felt incredibly natural from the get go, and even more intuitive than detach). This will basically be sugar over withTaskGroup(_:) – so the child task will keep the parent task's information and everything else we love about structured concurrency. With that, using detach will hopefully be a rare necessity – or maybe even barely a thing since we also have the @asyncHandler function annotation.

39

I'm on the side of body first, then cancel-handler. I don't have a strong opinion on the spelling. It's probably better to avoid repetition of cancel like in ktoso second exemple. I would say:

runTask {
  // task body
} cancel: {
  // cancellation logic
}
2 Likes
40

Would the proposal authors be so kind and comment on my previous message? I interpret the likes on it as if the people who left a like would also want to have these question officially answered.

To reiterate. I strongly think it’s not the responsibility of a brand new and general purpose type like Task to map 1 to 1 an existing set of priority from a different module which had those case names established during Obj-C time with the main focus around UI based applications. Task.Priority should have more general case names, some more generic terms of art, which would also fit other types of application, not only UI focused applications. Any other API that influenced this type should provide a ‘non-failable init to map the type into its own domain’.

To add on that:

  • I always found the user* case family confusing, as nothing indicates which one is more important.
  • I think there shouldn’t be a default “case”, it should have a more generic name. Instead the default should be a static constant which would point to the case that represents the default.
  • Is unspecified really needed? If it’s not specified then there is no priority. It feels like Optional<Task.Priority>.none would already naturally express that.
  • I think Logger.Level from swift-log is a great example where the API was build in a way to unify other applications and not just to support or map 1 to 1 Apple’s Logging APIs.
9 Likes
41

More importantly, I've seen a few of these proposals mention how important scheduling is, and how executors and tasks need to work together to do that.

AIUI, priority is the way that a task communicates metadata to its executor ("I'm a background task", or "I have something to do with immediate feedback to the user", or by extension, "I'm an audio task" or "I'm a physics simulation task"). It's purpose is to inform the executor's scheduling decisions. It really should scale beyond the immediate needs of UI applications on Apple platforms.

This is why I asked (and continue to wonder) about executors being able to access the TaskLocalStorage of enqueued jobs. If that was possible, you might be able to attach other kinds of priority information which custom executors could use.

2 Likes