[Second review] SE-0472: Starting tasks synchronously from caller context

Evolution Proposal Reviews
1

Hello, Swift community!

After the first review of SE-0472: Starting tasks synchronously from caller context, the Language Steering Group returned the proposal for revision.

During the review, an enhancement was discussed that would remove the restriction that the task's closure could not be isolated to a different actor, and the Language Steering Group agreed with lifting that restriction. With that change in behavior, it was also requested that a new name be chosen for the API, since execution of the closure would no longer be "synchronous" if it had a different dynamic isolation from the caller.

A diff of the changes from the first version of the proposal can be found here.

The second review of this proposal begins now and runs through May 19th.

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 me as the review manager by DM. When contacting the review manager directly, please put "SE-0472" 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/swiftlang/swift-evolution/blob/main/process.md

Thank you,
Tony Allevato
Review Manager

7 Likes
2

Overall I'm +1 on the proposal with the latest changes. I think this solves an important use-case for UI applications and in general makes unstructured tasks better to deal with. I have some clarifying questions on the proposal though.

Suspension points

After a suspension happens the task will resume on the the executor as implied by the task operation's isolation, as would be the case normally.

Upon first suspension inside the immediate task, the calling executor is freed up and able to continue executing other work, including the code surrounding the creation of the immediate task. This happens specifically when when a real suspension happens, and not for "potential suspension point" (which are marked using the await keyword)

These two sections are talking about suspensions but the proposal isn't clear what definition of suspension it is using. Often people are thinking about suspensions when a continuation is awaited. However, there is also the concept of enqueueing a task's job on the executor which can be seen as a potential suspension since another task can interleave. The latter often happens with isolation and task executors. This might be outside of this proposal but it might be good to officially define what a suspension point is at some point.

Example of an isolation change based suspension 
// Module A
@MainActor
func foo() {
  Task { @MainActor in
      MainActor.assertIsolated()
      print("Unstructured task")
      MainActor.assertIsolated()
  }
  Task.startSynchronously {
      MainActor.assertIsolated()
      print("Immediate unstructured task started")
      await yield()
      MainActor.assertIsolated()
      print("Immediate unstructured task finished")
  }
}

// Module B
public func yield() async {
    print("yield")
}

The above prints

Immediate unstructured task started
Unstructured task
yield
Immediate unstructured task finished

@isolated(any) closures

The newly proposed APIs don't mark their closures as @isolated(any). What's the reason for that? It wasn't entirely clear to me why those closures aren't annotated.

@discardableResult tasks

It was previously brought up, that the current Task.init and Task.detached methods are marked as @discardableResult that often lead to developers not storing those unstructured tasks which easily leads to unexpected behavior. Especially when the Failure is not Never then the failure will never be handled. With this proposal we are introducing another set of APIs that follow the same pattern. Should we reconsider the @discardableResult on those new APIs?

3 Likes
3

I am +1 with the latest set of changes.

I roughly followed the previous review and the changes to it in this iteration are a big improvement in my opinion. I feel like it is much more clear on intent and usage.

4

A suspension is a very specific thing and is when an async function actually suspends, i.e. gives up the carrier thread it was running on. I don't think we should be throwing in definitions in the middle of other proposals like this one, but I do agree we should probably update the swift book with proper docs on those terms.

I'm kinda hoping to do a bigger pass on the swift book, and we should do that there, i.e. "potential suspension point" vs "suspension" etc. But yeah I don't think we should be doing this do this here in the middle of "some" proposal for SE archeologists to try to find it.

The newly proposed APIs don't mark their closures as @isolated(any). What's the reason for that? It wasn't entirely clear to me why those closures aren't annotated.

They indeed did end up having isolated(any), as we were polishing up the exact semantics along with the new attributes powering it [stdlib] Adopt `@_inheritActorContext(always)` on `Task.immediate` by xedin · Pull Request #81572 · swiftlang/swift · GitHub So the signature ends up being @_implicitSelfCapture @_inheritActorContext(always) operation: sending @isolated(any) @escaping () async throws -> Success for Task.immediate specifically, for example.

@allevato I'd propose that what we should do here proposal wise is that, we could with modifications that we did add that isolated(any), that doesn't warrant another review/revision I think. I'll put up a PR to update the specific signatures we ended up with to the proposal so it can be used as source of truth.

This comes into play only when a specific isolation is provided to the closure, but indeed if it is, we should have same semantics if we have to hop, as a normal Task.init, so we do need that isolated(any) after all.

@discardableResult tasks

[...] that the current Task.init and Task.detached methods are marked as @discardableResult [...] Should we reconsider the @discardableResult on those new APIs?

As one of my good New Zealander friends would say... "yeah... nah." :slight_smile:

Should we be reconsidering the discarding? Yeah.

Should this proposal be doing that change only for those new funcs? IMHO, Nah.

Not loking the state of the stdlib is fine, but the top priority of libraries IMHO is consistency. If I'm using Task.init, Task.detached, and now Task.immediate I expect them to more or less function the same unless they have a specific functional difference about some aspect. The result discarding behavior of this API is not inherently different in this API. It is not inherent to this API that it is not discarding results; only the different execution semantics are the inherent difference. As such, it would be a messy to just randomly decide that this one API now will have some other behavior -- especially without having pitched this behavior change at all.

We can pitch that change about discardableResult on all unstructured task APIs, but then we should change all of them, and not sneakily change one of them on a whim like that.

Thank you for spending the time and effort reviewing the proposal!

6 Likes
5

There's a thing that is still bugging me a bit, that I had mentioned it in the previous review for this proposal and was also brought up in the review for SE-0475 (Transactional Observation of Values). Basically, are there any concerns about using Task.immediate to deterministically ensure a for await loop is awaiting new values?

For example in the SE-0475 review thread this sample code was drafted:

// Assume everything has the same isolation
func startObservation() {
  let values = Observations { ... }
  Task.immediate {
    for await value in values {
      print("Fresh value", value)
    }
  }
}

startObservations()
// <- Is 'Fresh value' already printed, here?
doSomethingElse()

And it was empirically determined (using a dev toolchain) that the print is indeed invoked in Task.immediate before suspending (and thus before startObservation() returns).

Is this behavior reliable? Do the authors foresee any issues that could arise by using Task.immediate in this way?

I know this is more of a question about how AsyncSequences work, but if this API ends up being widely used for this, its interaction with AsyncSequences will become very relevant.

Personally I was a bit surprised that the print was invoked before Task.immediate returned, I would have expected the "real" suspension point to happen after the loop was already ready to receive values but before the closure was called for the first value.

But if I'm not mistaken that depends on the implementation of the sequence. There's nothing in AsyncIteratorProtocol that enforces anything at all happens in next() before the first "real" suspension point. So, wouldn't different conformers of AsyncSequences exhibit different behaviors when used as in the code above? If so, couldn't this API (however useful for its intended use case) become a footgun when devs inevitably try to use it to observe AsyncSequences?

The reason I find it concerning is because I can imagine the same sequence of events that we saw in the SE-0475 review thread happening to many other developers:

  • Think of using this API to observe AsyncSequences.
  • Test if it works as expected.
  • It indeed works as expected with a test sequence.
  • Discover that it fails under slightly different circumstances (different type of AsyncSequence, different isolation...).

To be clear, I don't think there's anything wrong with the API per se, and I'd personally find it quite useful. It's just the potential for misuse that worries me.

6 Likes
6

This would indeed depend on specific implementations and even timing of entering the next() of an async sequence. This is in line though with any other suspension point's behavior, if something doesn't "actually" suspend we'd just continue running - which seems was the case in your example.

I will not though that snapshot toolchains don't have the complete implemetation yet.

This is completing the impl and will be merged any moment now: [stdlib] Adopt `@_inheritActorContext(always)` on `Task.immediate` by xedin · Pull Request #81572 · swiftlang/swift · GitHub given that seems we're converging on agreeing on this proposal.

7

This proposal has been accepted with modifications.

2 Likes