21

It exactly is a “preference” and not a “requirement” though.

Please refer to the diagram in Proposed solution to see in which situations the preference is overruled by actual requirements (an actors custom executor).

2 Likes
22

Oh no, yeap this sadly prevents this approach, and we'd have to make separate overloads for picking an TaskExecutor and for the other preferences. Not the end of the world but rather unfortunate... I should give these a spin and see how it feels in practice.

A real bottom-type Never would be so lovely to have someday.

23

I am suspicious of using Never for this. Never is useful for type-level assertions. The canonical example is Result<_, Never>, which uses the type system to prove that .error() case cannot be instantiated.

Does a Task where ExecutorPreference == Never mean a task that cannot be executed, since no Actor with Executor == Never could ever exist?

5 Likes
24

i wonder if there is any metadata we’re currently expecting to be able to get from an arbitrary AnyObject.Type? if not perhaps we could just special-case Never to be able to satisfy AnyObject.

25

The ability to refer to the "global concurrent executor" some way other than nil would be nice. It could also serve as a nice place to house some documentation. You could use the SwiftUI technique of self-constrained extensions

extension TaskExecutor where Self == DefaultConcurrentExecutor {
    var defaultExecutor: DefaultConcurrentExecutor { .shared }
}
Task(on: .defaultExecutor) { }

Since, as you pointed out, explicitly specifying "inherit preferred executor" is potentially dangerous, I think this could be a nice approach. It just substitutes nil for .defaultExecutor. Assuming DefaultConcurrentExecutor.shared is okay to add to the StdLib :D

That said, nil => "no preference" => "default behavior, global executor" is fairly intuitive I think. I would maybe change the argument name from just on executor: to on preferredExecutor: just to make it very clear when you're reading docs/autocomplete.

1 Like
26

Small update on the thinking around the discussed here options.

We thought about this and think that if we made the global default executor available as a task Executor, then the preference can be spelled explicitly as:

withTaskExecutor(.defaultConcurrent) { ... }

or something like this; so we avoid having to invent a "fake" spelling for the executor. I'll investigate this approach some and provide an update.

Meanwhile, I would request/suggest focusing on the runtime behavior aspects of the proposal, as that's the "big" feature here, so it'd be good to hear any concerns if anyone has some to share. We thought the inherited preference behavior through a lot, but it'd be good to hear if this behavior is intuitively understood by reviewers (I'd hope so, it tries to stick to the nature of structured concurrency task trees after all).

2 Likes
27

One of the first thoughts I had was "Oh, could this replace PointFree's replace-the-global-enqueue-hook hack for more deterministic testing?"

// Naively run all test tasks on the main thread…
await withMainExecutor {
    await testMyCode()
}

Testing wasn't mentioned in the proposal, but I'm sure I won't be the only one to think of this.
Also, with the exceptions mentioned in the proposal, it seems like it might not work completely?

I guess I'm wondering if you think this is a viable solution to this topic from last year since the approach seems somewhat similar.

1 Like
28

Looping back here after new year's break :wave:

Not sure what the "completely" you have in mind here is? No it won't affect "all" tasks if that's what you mean.

The feature affects structured tasks, so yeah all structured tasks could be forced onto some specific queue -- if you make a custom task executor, and implement it using the main queue.

We don't provide the main queue executor as task executor in this proposal out of the box. Arguably the main thread is the one that you'd actually not want to use with such sticky behavior -- because you can/should use the usual way to guarantee this execution, the @MainActor annotations. But as you said, it's possible, by writing such small queue/executor shim. It could be argued either way though and such executor is possible to write / provide if we needed to someday.

Hooking the global enqueue hook is a rather blunt tool, and as I commented in other threads, setting and un-setting the global hook is bound to cause trouble eventually. If it is done only once at program start it's safe but the way I've seen some people use it is definitely not safe.

29

Review update: I'm investigating the shape of the task initializers and the on: parameters -- to see if we can offer the "default global concurrent pool" as a task executor, which would simplify the initializers and withTaskExecutor shapes perhaps.

So this is how I believe we could address the feedback shared so far in the thread:

Short version:

  • initializers and with... methods change from (any TaskExecutor)? to some Task Executor
  • we introduce the a DefaultConcurrentExecutor which delegates to the existing underlying shared global default thread pool
  • this allows to spell the "disable preference" as withTaskExecutor(.default)

Details in the links above.

3 Likes
30

What is the semantics of the UnsafeCurrentTask.unownedTaskExecutor - does it indicate what preference is set, or which executor is really running the job?

What would this code print?

@MainActor
func checkExecutor(_ executor: NaiveQueueExecutor) {
  withUnsafeCurrentTask { task in
    print(executor.asUnownedTaskExecutor() == task?.unownedTaskExecutor)
    print("Is on main queue?")
    dispatchPrecondition(condition: .onQueue(DispatchQueue.main))
    print("Yes")
    print("Is on executor queue?")
    dispatchPrecondition(condition: .onQueue(executor.queue))
    print("Yes")
  }
}

@main struct Main {
  static func main() async {
    let q = DispatchQueue(label: "queuename", attributes: .concurrent)
    let executor = NaiveQueueExecutor(q)
    let t = Task(on: executor) {
      await checkExecutor(executor)
    }
    _ = await t.value
  }
}

As far as I can see from the current implementation, it returns the preference, not the executor that was used for scheduling.

true
Is on main queue?
Yes
Is on executor queue?

💣 Program crashed: Signal 5: Backtracing from 0x183316d0c... failed

So the precondition() example in the proposal is not reliable:

precondition(currentTaskExecutor == eventLoop.asUnownedTaskExecutor())
// perform action that is required to run on the expected executor
31

It returns the preference. The behavior you show is expected — follow the diagram at the top of the proposal where it illustrates “where” code executes — since this is a method on an actor with a specific executor requirement it will run on the actors serial executor. The “main actor” is not a “default actor”, it effectively has a custom executor basically — that is the main queue, so the preference is overruled by this strong requirement.

The checking like that can only ever work in nonisolated async functions, where the preference does matter.

32

I'm not certain that this is an overall improvement. Particularly with TaskGroup.addTask, there really is a meaningful distinction between "no preference" (i.e., inherit whatever preference the current task has) and "run on the global concurrency pool". The former is quite naturally expressed as nil, so long as the argument label is something meaningful, e.g., addTask(executorPreference: nil).

Under your proposed amendment, which overload of addTask gets called determines whether you inherit or not. That will work, but I have a few concerns about that being the approach:

  1. There's value in being able to explicitly write that we're adding a task without an executor preference, e..g, for documentation or auditing purposes. Omission of an argument is a subtle way to do this, whereas the aforementioned addTask(executorPreference: nil) { } makes it clear that you thought about it and chose not to state a preference. addTask() { } is more subtle.
  2. The fact that we have two overloads is really an accident of history, because this feature is coming a few years after addTask was introduced. We wouldn't even have overloads here if not for the availability difference, so it feels odd to rely on them being different.
  3. If we need to add another addTask overload for some other reason next year (e.g., to add one more optional parameter), we would have to add two new overloads---one with an executor preference, and one without. This doesn't scale well for future evolution.

Now, I can understand the desire to replace the existential any TaskExecutor with an opaque parameter some TaskExecutor: that is generally the direction we tell folks to go, and I've pushed for it as well in a number of places. However, we're not buying ourselves much with this change, because the way this feature is implementation effectively requires that we immediately type-erase this value.

Personally, I think the (any TaskExecutor)? formulation in the original proposal was better, and that the only improvement we need for the parameter is a more descriptive argument label.

I certainly do understand the desire to have a way to spell the "global concurrent pool"; that was missing functionality from the original proposal, and it's useful to be able to say explicitly (especially for the addTask operation of task groups) "run this on the global concurrent pool." In a world where we're passing existentials task executors into addTask, Task.init, and so on, I don't think we need the DefaultConcurrentExecutor type to exist: we only need a global variable:

var globalConcurrentPool: any TaskExecutor

and, yes, I'd like us to consider whether we can make that assignable (with whatever runtime restrictions are needed) now or in the future, so it's possible to drop in your own concurrent pool in at the application level rather than overriding the C++ hooks like one does today.

Doug

10 Likes
33

Agreed, seems we overcorrected there a bit - thanks for chiming in, Doug.

As far as naming, yeah the "on" has indeed become rather confusing and we need to go the explicit route. I thought about this a bit and can't come up with anything better than explicitly naming the property of the task this is setting, so:

Task(executorPreference: ...)
group.addTask(executorPreference: ...) { ... }
await withTaskExecutorPreference(...) { }

While verbose, I do think it is well worth it here since this is more than just starting the task on a given executor, but the "preference" aspect is rather important. Both because it is not a strong requirement, and because it affects the entire task tree -- so giving people a good name to search documentation / google for is worth it here.

--

I like venturing towards a default executor global value like this -- making it mutable in the future would indeed allow swapping the default executor nicely. ABI wise it seems making it mutable in the future will be okey, so we can just keep it a get-only for now, and add the setting when we're ready for it. We may have to add more protocols there in the future: var globalConcurrentPool: any TaskExecutor & SomethingElse but I think that'll be ok.

Naming wise for the property I'd probably try to avoid introducing the "pool" noun here, as we don't really discuss thread pools etc in Swift Concurrency terminology... There may be a "...Pool" type someday and the naming would conflict weirdly -- on the other hand, we could then deprecate this property...

Long story short, I'd probably call it var defaultConcurrentExecutor -- open to other ideas though.

I should also mention that this touches upon a different proposal/work that we had started but not finalized yet with @etcwilde a while ago -- customizing the MainActor's executor. Though there I think we ended up not with a settable global but with a static factory method... It would be good to have the two use the same style if possible -- I'll double check what our problems there were with a global settable var.

4 Likes
34

I'm fine with not using "pool" terminology, but I'm not a fan of the "default" here, because it's not a "default" in all contexts. The default is inherit (for structured tasks) or use the global concurrent executor (for unstructured tasks). I think "global" is better.

If we're likely to have to change the type signature for this global variable when/if we allow replacement of the global concurrent executor, I get a little worried because that's a foreseeable, future breaking change. I don't necessary know what to do about it, though. It's useful to get access to the global concurrent executor now, and we don't quite know what it would take to replace the global concurrent executor in this manner, so neither subsetting out this functionality nor expanding the proposal to include replacement is obviously the right choice.

Doug

2 Likes
35

Perhaps it shouldn't be a let or var, then, but rather a getter function. That way adding mutability is simply adding a setter function, and while it might raise the question from latecomers as to why it's not simply a var, it's at least neat.

As opposed to having a let now and then having to either replace it with a var (migration pains) or add a setter function, which is a less natural pairing.

36

I'm not sure what refinement we would want to add to the type of the global executor. I think we'll eventually need some more requirements on the Executor protocol to optimally support priority escalation, but those will, well, probably need to be on the Executor protocol, because they apply equally well to actor executors.

1 Like
37

Yeah, global... sounds good -- I'll reflect that in the proposal :+1:

On this topic I'll check if the var approach will be okey to make settable in the future.

Type wise you're probably right John -- the future changes would probably be around adding more requirements, and not necessarily changing the type, so we'd be okey with this API shape :thinking:

Thanks for the input everyone!

1 Like
38

I put some thought towards the optionals and nil discussion.

It is a bit nuanced, however the core way to understand it is that "nil" is "this task (tree) has no specific no preference", so in structured concurrency it'll inherit and in unstructured passing nil also doesn't change semantics -- both cases are a no-op, but can serve as documentation, or makes it easier to obtain the preference from configuration and pass around etc.

I was going back and forth about allowing the optional in withTaskExecutorPreference since it's a no-op if nil is passed. However, for the same reason as in Task() -- for passing an optional preference around, I eventually decided to allow an optional in this API. It makes it easier to have an optional preference and just pass it around everywhere, rather than suddenly have to if let and have separate code paths when withTaskExecutorPreference is used.

I also introduced the global globalConcurrentExecutor variable.

The revisions are reflected here:

And implementation PR (which also adds a lot of docs): [TaskExecutors] Task initializer and withTaskExecutor parameter changes by ktoso · Pull Request #70783 · apple/swift · GitHub

edit: I should probably spell out that the way to say "do NOT inherit, use the global concurrent executor" is to explicitly pass the global executor: addTask(preferredExecutor: globalConcurrentExecutor) that's all covered in depth in the proposal update.

5 Likes
39

Hello all,

The Language Steering Group has decided to extend the review of this proposal by another week (to January 23rd) to discuss the set of changes the author has provided:

  • Consistent use of optional task executor preference throughout the APIs

  • More descriptive name for the task executor preference argument

  • Introduction of the global concurrent executor variable.

    Doug
    Review manager

2 Likes
40

It sounds like the review period is over, but I still wanted to hop in and say I appreciate the thoughtful discussion and revision. I think the API design is in a solid place. Looking forward to this!

2 Likes