[Concurrency][Pitch] Task Local Values

[ktoso (Konrad 'ktoso' Malawski 🐟🏴‍☠️)]

Thanks a lot for the review, Chris - great to have some in-depth discussion on this

@Chris_Lattner3

[...] better late than never. If this is too late, then I can bring up similar points in the formal review threads.

It's just on time, as this was first posted to highlight missing things in the Task proposal (which you also notice in this review ) and since this was posted the missing things we ironed out in Task APIs and the upcoming Structured Concurrency Pitch #3 will have all APIs which enable this proposal to be implemented properly (i.e. also working from within sync functions).

At the time of writing this in December it wasn't clear what will happen with the Task / unsafeCurrent APIs if they'll happen or not, so the linked complete implementation and pitch were not able to implement the full potential of this pitch yet -- thankfully now soon we'll be able to as the calling convention and ABI of tasks/executors is changing to manage a thread local for the Task so it can be used for Task.unsafeCurrent which enables great updates to these APIs).

So timing is good, thanks for chiming in, let's dive in...

---

Usage in synchronous functions (coming very soon)

@Chris_Lattner3

Task local values may only be accessed from contexts that are running in a task: asynchronous functions. As such all operations, except declaring the task-local value's handle are asynchronous operations.

Is this imprecision in the wording, or do you mean this literally? Synchronous functions can be called from async functions and would like to access task-local values just like async functions do. [...]

I don't think it is acceptable to punt this to Future Directions . This is core to the proposal, and the proposal isn't very useful without solving this.

Yeah, I absolutely agree!

Back in December when this was implemented and pitched I had my hands tied somewhat by being unable to implement this given the restrictive state of the Task APIs. Access to tasks is currently implemented as "magical extra parameter to async functions," making it impossible to implement accessing a task from a sync function. It is indeed a huge, feature-breaking even, limitation...

Thankfully, during Pitch #2 of Structure Concurrency in the last weeks (this design) we were able to hash out the missing and misaligned APIs which resulted in: Task as struct, static non-async funcs, and UnsafeCurrentTask by ktoso · Pull Request #48 · DougGregor/swift-evolution · GitHub which will be Pitch #3 of those APIs.

This implies changes in ABI/calling-convention how a Task is passed around, so it's still not implementable until that is changed, but at least the team seems to be agreeing on adding Task.unsafeCurrent so finally we can express task locals within synchronous functions and this proposal becomes wholly useful then

@Chris_Lattner3

I don't understand this at all. The only reason for a function to be marked async is if it suspends. Access to a task local value never does that, so I don't see why it would be marked async.

The writeup above I hope answers this: this was the only way to implement this. It sucks, and it will be fixed in Pitch #3 with introduction of Task.unsafeCurrent

Indeed, Task.local(\.x) becomes non-async then

---

withLocal { ... } spelling

@Chris_Lattner3

await Task.withLocal(.requestID, boundTo: "1234-5678") { await printRequestID() // 1234-5678 } await printRequestID() //

I understand your design point of being for library authors (e.g. like the internals of S4TF) but this seems really awkward to use in practice. Why not have a more direct getter/setter style of API, possibly wrapped up with keypaths or other nice things to make them beautiful?

@Chris_Lattner3

Why can't I just have unstructured access to a task-local value with a get/set value style of API? You're already handling the case where you get a value that isn't already initialized.

Reads are trivial and solved, as you say.

It is the "binds" that must be structured. The reason it can't be "just a setter" you kind of said yourself -- it would not be structured.

As usual... with enough compiler magic, everything is doable, and we could implement it as:

func foo() {
  // yeah we could even do Task.local.foo = 222, 
  // I did exactly such API for swift-distributed-tracing's attributes;
  Task.local(\.foo) = 222 // "pushes" a binding to task-local bindings stack

  // every binding magically generates:
  // @synthesized defer { Task._popTaskLocalValue() }
  // which must "pop" the binding off the task-local bindings as we return

  assert(Task.local == 222)
}

Task.local(\.foo) = 111
foo() // NOT a child task; foo is 222 inside there.
assert(Task.local(\.foo) == 111) // this assertion MUST hold

So while this may be "less noise" it is definitely a lot more magic and surprising: we're writing to something that looks static, but it'll automagically isn't and unbinds at the end of the scope...

Whereas the with style has lots of prior art and feels like pretty normal day-to-day Swift. There is no need to explain any magical semantics when presenting the following code:

func foo() {
  let before = Task.local(\.foo)

  Task.with(\.foo, boundTo: 222) {
    assert(Task.local(\.foo) == 222)
  }

  assert(Task.local(\.foo) == before)
}

This both enforces the right semantics, and informs the programmer with the shape of the API what's going to happen here. Is pretty much "normal" swift code and no-one has to jump through learning the specialized magical semantics of scoped task local values to understand this. Therefore, I think, the body/scope of withLocal(...) {...} is not noise, but helpful information.

So I really believe the less magic version is much preferable here, because it is more true to what is actually happening.

"currentActor"

While actors are out of scope for this proposal, it should also be possible to get a pointer to the current actor (or nil if not on one) from sync code as well for the same reason.

Yeah that's absolutely useful and probably one of the main things we'd spend time on when instrumenting Akka It helps a lot in debugging/logging and just understanding what's going on in the system.

+1 overall, but let's keep this separate from this proposal. I do suspect though we'll end up having such thing. Probably through the Task.current APIs

---

@Chris_Lattner3

The Unsafe part of the name originates from the fact that it is NOT safe to modify such task object from other tasks, and such developers must not pass such task object to other tasks.

This is also concerning to me. Why are you conflating mutability into this? Swift has well defined ways to handle mutability, why do we need another one? There should be nothing unsafe about getting an opaque task handle. Such a thing is the basis for a lot of functionality (e.g. debugging and reflection APIs etc).

That's a specific request from @John_McCall -- this isn't really about the task local values but about the general existence of an "give me a handle with full access to everything, even APIs which should never be called from a different task". We can get into the details of that Unsafe... name and type in the Pitch #3 of Structured Concurrency which should be posted this week I hope.

The actual unsafety of it comes not really from the Task.unsafeCurrent but the fact someone could escape/store this value and then use it from a different task, that's the unsafeness we're concerned about. John wanted to eventually make a type that's "un-movable" which then could be safe, since we'd prevent people from doing this. ConcurrentValue things don't really solve this -- people could store it in some property and then touch the same property, get that UnsafeCurrentTask from a different task and poke at it unsafely.

@Chris_Lattner3

Task local variables are semantically inherited the same way by child tasks similar to some other properties of a task, such as priority, deadline etc. Note that when storing reference types in task-local storage, they can be read by child tasks of a task, meaning they should SHOULD be either immutable, or thread-safe by some other means.

Please refine the proposal to tie into the ConcurrentValue proposal here. We need one safe and consistent way to handle/reject transfers across concurrency domains.

Will do, by now the ConcurrentValue is fleshed out enough and looking good that this seems like a good time to refer to it. I agree that restricting values stored in task locals to ConcurrentValue is a good idea

---

@Chris_Lattner3

It is important to keep in mind the intended use case of this API. Task local values are not indented to "avoid passing parameters because I'm lazy" because generally the implicit propagation of values makes the application harder to reason about.

Just a suggestion on the writing, but I don't think it is productive to imply laziness. There are perfectly legit reasons to use "global" or "task local" state. It is also (factually) a thing that would be abused in some cases, but if you're going to try to address this concern, then please do so in a way that is more based on technical concerns than a character insult.

Thanks for the catch here; my self-deprecating humor backfired here, will reword this -- thanks for pointing it out

Please let me know if this makes sense -- in the meantime I think it's time to prepare Pitch #2 here and base it on the existence of the upcoming Task.unsafeCurrent