SE-0304 (2nd review): Structured Concurrency

Evolution Proposal Reviews
1

Hello, Swift community.

The second review of SE-0304: Structured Concurrency begins now and runs through April 16th, 2021.

The first review received largely positive comment, but led to a number of revisions, particularly regarding naming of methods. You can find a diff for the revision here .

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:

https://github.com/apple/swift-evolution/blob/master/process.md

As always, thank you for contributing to Swift.

Ben Cohen
Review Manager

17 Likes
2

Looks like the diff link is linking to the diff of a different proposal?

3

The view may be a bit confusing but it shows the right diff — please click “Load Diff” on the linked to proposal file and you’ll have the diff of the exact document in view.

1 Like
4

Yeah, if anyone knows a way to create a link to a single file's differences between two commits on GitHub, LMK :)

1 Like
5

Welcome to another review round everyone :slight_smile:

——

I think we’re pretty happy with the changes in names and lifting a lot of functionality out of the Task namespace into top-level. We also improved usability of groups a bit, though they remain the low-level building block that they are.

Please have a look — the summary at the bottom of the revision update contains all renames in one list for a quick overview.

—-

Two minor things we wanted feedback on:

withTaskCancellationHandler / withCancellationHandler?

The changed top-level cancellation handler functions are now called withTaskCancellationHandler(handler:operation:) we're not sure if the Task word in there is helpful or not. It is a fairly long unique name, so perhaps no need to qualify with Task? On the other hand, with Task it is clear what cancellation this is talking about.

Order of parameters in cancellation handlers?

When writing some more cancellation handlers this week we asked ourselfes if the order is nice or inversed would be nicer?

Today’s API reads like:

withTaskCancellationHandler {
  print(“Cancelled”)
} operation: {
 print(“work”)
}

I was pondering if it wouldn’t be nicer if the operation was first, though wanted some feedback from the community on this:

withTaskCancellationHandler { 
 // work
} whenCancelled: {  // or onCancellation or some other phrase?
  print(“Cancelled!”)
}

Thanks in advance,

7 Likes
6

Maybe

withTask {
  ...
} onCancel {
  ...
}

SwiftUI's naming scheme on event handler (on + Verb) fits pretty well here.

19 Likes
8

Task can de dropped. As a would-be user, I just need to know I am waiting and using a cancellation handler. Also, it just sounds better.

However the order should really be reversed. What am I waiting on (first) and what needs to happen when the task is cancelled (second).

I only followed this proposal in passing. Not a native English speaker.

3 Likes
9

Your first suggestion would potentially be confusing because it ends both times with cancel.

10

Yes, the first one is kind of confusing.
So I'm preferring the second one.

11

I think with cancellation handler follows the normal ordering for multiple trailing closure here, where the main closure goes first, followed by additional closures.

The main closure would be the actual work itself, with cancellation attached as an addendum.

4 Likes
12

I think I misunderstood the original withTaskCancellationHandler(handler:operation:).
@Lantua 's suggestion is more accurate.

withTask {
  // work
} onCancelled: {
  // cancellation handler.
}

or

withOperation {
  // work
} onCancelled: {
  // cancellation handler.
}
13

Nit: there seem to be a few spots in the document where @concurrent is used instead of @Sendable. For instance:

/// Create a new, detached task that produces a value of type `T`.
@discardableResult
static func detach<T: Sendable>(
  priority: Task.Priority = .unspecified,
  operation: @escaping @concurrent () async -> T
) -> Task.Handle<T, Never>

should be

/// Create a new, detached task that produces a value of type `T`.
@discardableResult
func detach<T>(
  priority: Task.Priority = .unspecified,
  operation: @Sendable @escaping () async -> T
) -> Task.Handle<T, Never> {
14

Thanks — I’ll do a another pass looking for those remnants. After working with both for such a long time they mentally merged into the same thing in my brain and eyes...

Thanks for spotting this, indeed @Sendable is the thing nowadays.

(Also happy to accept a PR with such fixups if you’d like @nikitamounier to send one in, that’d be very nice!)

1 Like
15

Sure, I'd be happy to do that. There seem to be a couple more remnants from the old proposal (especially now that many of the functions aren't static Task extensions anymore), but it should be pretty trivial to compare with the latest version on the Swift repo.

16

It may be a dumb question, but why is the API shaped like this:

withTaskCancellationHandler {
    // cancel started work
} operation: {
   // work
}

and not like this:

Task.onCancelled {
    // cancel started work
}

// work

The example from the proposal:

var urlSessionTask: URLSessionTask?

return try withTaskCancellationHandler {
    urlSessionTask?.cancel() // runs immediately when cancelled
} operation: {
    return try await withUnsafeThrowingContinuation { continuation in
        urlSessionTask = URLSession.shared.dataTask(with: url) { data, _, error in
            if let error = error {
                // Ideally translate NSURLErrorCancelled to CancellationError here
                continuation.resume(throwing: error)
            } else {
                continuation.resume(returning: data)
            }
        }
        urlSessionTask?.resume()
    }
}

could then become:

var urlSessionTask: URLSessionTask?

Task.onCancel {
    urlSessionTask?.cancel() // runs immediately when cancelled
}

try await withUnsafeThrowingContinuation { continuation in
    urlSessionTask = URLSession.shared.dataTask(with: url) { data, _, error in
        if let error = error {
            // Ideally translate NSURLErrorCancelled to CancellationError here
            continuation.resume(throwing: error)
        } else {
            continuation.resume(returning: data)
        }
    }
    urlSessionTask?.resume()
}
1 Like
17

You require a scope that the cancellation handler can be triggered, hence an operation to go along with it. Scoping the handler to the current task is unstructured as it's still applied even after the function returns.

In your code, the task can be cancelled after the URL session finished fetching data, which is benign for URLSession (I believe), but may cause problems on less robust systems.

18

Correct, yes. The handler must be set for a specific scope — the duration of the operation.

Think of this like a stack of handlers, which are added as scopes are entered. As a scope is exited, the handler is popped/removed. Notice that the handler closure also can close over state, so if they would live forever like with the Task.setHandler style, they would very very easily lead to leaks (or super annoying weak dances).

The structured way to execute a scope with a handler ensures that we can: efficiently allocate them using task local allocation (not “task locals”, just like a task-local alloc), and easily and safely (without heavy locking) remove the cancellation handler as the scope ends.

So it’s pretty important to keep that scoped “shape” if it for those reasons.

19

This probably already has been discussed somewhere, but can someone point me at or tell me what's the reasoning behind copying the same names for Task.Priority from other Apple API's other than precedence? Not every application has UI involved, yet the naming scheme still uses that convention. If there is still room for improvement, I would personally love a new more generic set of names. The frameworks that need a 1 to 1 conversion can provide a non-failable initializer on appropriate types. Dispatch framework originated at Apple but was made available for other platforms, so there was no room for changing the names. However Task a brand new API which is meant to be a general purpose API, so why do want to re-use the naming convention from a framework which was based around UI?

20 Likes
20

I uploaded the PR – if anyone notices anything else I might have missed feel free to add a comment.

21

Since you're asking for opinions, I think Task isn't helpfully redundant within withTaskCancellationHandler, preferring withCancellationHandler, primarily because it's not differentiating from any other cancellationHandler in the API that I'd otherwise possibly confuse it with.

Likewise in the order of parameters, I tend to think of the operations I want to do first, and the cancellation handler second. No more reason than it feels more comfortable to me that way, but in either case I wanted to put another "I like that" vote by onCancellation. I think that pattern of naming makes it exceptionally clear that the closure will be invoked when a task is cancelled.

4 Likes