This proposal is part of the larger concurrency feature. There have been quite a lot of these reviews, and there are a quite a few more coming, I'm afraid; still, we seem to have gotten through most of the more difficult ones. Thank you for your patience with all this.
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. If you do email me your feedback, please put "SE-0311" somewhere in the subject.
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
How married are we to using a type as a key? Because my first thought when seeing the title was to imagine something like:
@TaskLocal var requestID: String = “<no-request-id>”
And even after reading the proposal and seeing that we don’t want an ordinary setter, I think we could provide the desired semantics with a property wrapper along these lines:
// Using a class so it has a stable identity.
@propertyWrapper public final class TaskLocal<Value> {
var initialValue: Value
// Note: could overload with additional parameters to support other features
public init(initialValue: Value)
public var wrappedValue: Value {
get
@available(*, unavailable, message: “use ‘$myTaskLocal.withValue(_:do:)’ instead”) set
}
public var projectedValue: Self { get }
public func withValue<R>(_ valueDuringBody: Value, do body: () async throws -> R) async rethrows -> R
}
(On the other hand, this does have more potential for mischief: if you used @TaskLocal outside of a global or static let, you would create task-local variables with restricted lifetimes since the instances could be deallocated. We might not want that.)
It would be nicer to declare that’s for sure. But as you noticed, it allows for some wrong usage which is why I strayed away from that design.
Since you could declare many instances of what was intended to be an unique key things can’t end up very wrong. As you said, since the identify depends on the specific instance of the requestID then, so code like this:
actor Greeter {
@TaskLocal(<options>) var requestID: String = "<no-request-id>" // wrong!
func greet(other: Greeter) async {
await requestID.withValue("1-2-3") { // "ObjectId(self.reqestID) = 1-2-3
await other.hi("Alice")
}
}
func hi(name: String) {
// oh oh...! "other" requestID... so the value is <no-request-id>!
// since ObjID(self.requestID) != ObjID(the-actually-set-requestID)
print("Hello \(name) [requestID:\(requestID)")
}
}
is wrong because for the identity of the task-local we'd have to use the identity of the wrapper (class). So such subtle mistakes are possible.
I am super worried about such getting it slightly wrong. But perhaps this is a feature specific enough to warrant "you should know what you're doing"?
Especially because to be honest I don't expect people to write tests for these things because they most often are used for logging or tracing etc, which people often ignore in their testing regiment...
As you said, this would work if we stated the following rule: "A @TaskLocal value MUST be declared as static or global property". We could compiler enforce this... But it's a bit ad-hoc treatment of one specific property wrapper so not sure we want to do that...
You're also right we would keep the ability to customize the keys even with the wrapper... so that's good (and an important requirement).
• 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?
Yes.
• If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I was primarily interested in using this for distributed tracing with cross-services spans with up to a dozen hops, and was more than happy to see it as one of the use cases highlighted. Awesome.
We’ve done such tracing with TLS and/or explicitly sending contexts around (having to explicitly transfer it at thread context switch boundaries, something that seems to come for free with the "inheritance" to child tasks), I believe this proposed infrastructure would be much better suited for the reasons outlined in the proposal - especially if the “function wrappers" alluded to actually will come through in the future, that would be really awesome, as in general a significant effort needs to be put into such tools when developing distributed systems - having most of it out of the box is to use a technical term... kick-ass.
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I did a fairly quick reading and spent some time going back checking on details of interest.
Could this take a collection too instead of chaining? Perhaps as long as the collection is homogeneous on Key.Value ?
How do I get all local values with out using reflection to get all the keys? The use-case I am thinking is a logger context of things like region, hostname, etc dumped as JSON.
I would love it if the keys had a field for tags/groups so I could get all keys that belong to a group.
Not sure if that is the exact usecase for this api but these are my thoughts.
This addresses one of the bigger frustrations I've had going from Ruby on Rails to Node, which is that async code makes things more complicated to pass around. For instance, in Rails each request is automatically given a db transaction and through thread local values passed implicitly to each db call. This is automatic and requires no effort on developers part and thus "just works" even if the dev knows nothing about transactions. In node, that has to be explicitly passed around, and as a result most Node projects I've worked on don't bother using transactions.
Go's approach to explicitly pass a context around works well enough since you don't need to be explicit about every value, but that type of verbosity is common in Go but wouldn't fit well in the Swift ecosystem.
I love that this is addressing common issues with other implementations of this kind of thing.
I think the mechanism of using something similar to the SwiftUI environment is spot on. In Node, it's common to attach this kind of thing to the request object. Given Javascript's freeform nature, you can just add properties to any object. But that doesn't fit well into a typed language, especially a language like Swift. This approach gives us similar power and flexibility while maintaining type safety.
I'm a developer without much familiarity with this kind of feature in other languages, so if I'm misunderstanding anyway please correct me here! But the proposed syntax seems a bit overkill to me, for what it does.
On Environment as prior art
The proposal refers to Environment as prior art for the pattern. The Environment pattern has it's downsides - the verbosity of declarations is the obvious one, but also increased learning curve (the need to discover and remember both a protocol AND a ‘hidden’ type to extend), and indirection when jumping to definition in Xcode. These downsides would also apply here.
But in contrast to this feature, Environment comes with a big range of ready-to-use values, and new additions are a case of ‘write once, use everywhere’, so realistically you could enjoy the benefits many times over without ever declaring an Environment value yourself. Whereas this feature seems to be more for ‘write once, read once’ just for one-off custom values?
On the choice of KeyPaths over types
The proposal notes the setter isn't needed, and as far as I can tell, neither is the getter.
eliminating the need to write an extension with an unused getter, and reducing learning curve to using the feature? It trades off aesthetics in the business logic, but it it saves writing out a whole boilerplate extension, that would be a worthwhile tradeoff for me at least.
On Key-less value definitions (the considered alternative)
My suggestion is that the proposed solution, with protocol + extensions has some quite significant drawbacks - learning curve, verbosity, and indirection as listed above. So I'm looking quite keenly at the alternatives.
extension Task.Local {
var foo: String { "Swift" }
}
Going through the concerns listed for this alternative one-by-one, it still looks like the better solution:
it prioritizes briefity and not clarity. It is not clear that the value returned by the computed property foo is the default value. And there isn't a good place to hint at this. In the ...Key proposal we have plenty room to define a function static var defaultValue which developers need to implement, immediately explaining what this does.
Both options are going to have a hard time explaining the purpose of the getter - in the proposal, it's not clear if the getter is called, or what happens if there's two with the same key. And really, the behaviour of this alternative seems quite intuitive - the returned value is used, unless you choose to override it using task.local(_, boundTo: _)? I can't see how someone could use this feature incorrectly.
this shape of API means that we would need to actively invoke the key-path in order to obtain the value stored in it. We are concerned about the performance impact of having to invoke the key-path rather than invoke a static function on a key, however we would need to benchmark this to be sure about the performance impact.
Fair concern, but worth investigating. Could the compiler optimise away the TaskLocalValues empty-struct-init, similar to Void, meaning there'd actually be no overhead?
it makes it harder future extension, if we needed to allow special flags for some keys. Granted, we currently do not have an use-case for this, but with Key types it is trivial to add special "do not inherit" or "force a copy" or similar behaviors for specific keys. It is currently not planned to implement any such modifiers though.
Supposing we do go with this future direction - could these equally be parameters on the task.withLocal method?
I like the direction this is going, but I think it needs more iteration. In particular, it looks like the writing got updated a few times along the way as other evolution proposals came in and is now inconsistent in several places (more below). This makes it difficult to understand what is being proposed.
This is a key part of the Swift concurrency story, and many aspects of this area great: I like a clear designation of task local values as being separate from thread local and other preceding technologies, tying into structured concurrency and presenting the associated structure as a dynamic scoping mechanism is good, and many of the details and framing issues are very nice. I also like how this is clearly learning from prior art in other systems.
That said, I'd like us to continue exploring a few things:
The biggest and most important to me is that proposal makes great points about the analogy between TLS and builtin task properties like priority and 'is canceled', but then breaks the model for TLS by making TLS only accessible from async functions. I think it is very important for non-async functions to have access to TLS values, because "needing to suspend" and "needing access to TLS" are orthogonal features. Allowing sync methods to access TLS allows builder patterns and other things that are very common, and are otherwise implemented with thread-local systems in other languages, and in Swift 5. My understanding is that the runtime has evolved since this was first written, and this is not an implementation problem anymore.
==> It might be that the proposal has already switched, the writing is confusing and inconsistent on this point. This is what I'm reacting to in particular: "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."
Why does this feature add complexity in order to allow default values? Swift has very strong support for optional types. I would think that you'd build on top of that (having the lookup return an optional T), instead of building an alternative implementation strategy with defaultValue. If this was removed, then we could go with other approaches that require less verbosity/boilerplate to declare keys.
The implementation approach is quite sensible (I'm pleasantly surprised to see this level of detail in the proposal, thank you!). FYI, this algorithm is known as "path compression", at least in union-find like algorithms.
As a minor point, I'm curious whether the implementation will use inline optimization to optimize for a few task local values, or whether everything will use out of line indirect access. Indirect TLS affects memory use of tasks and the indirections affect locality. It seems worthwhile to provide some scratch space on the task object to make the "small TLS" case fast in practice.
The Task.local(..) and Task.withLocal(..) names don't connote getting and setting TLS values to me. There is a verb that is missing here, and I don't think the word "local" is specific or communicative enough.
" The handle is bound to a specific declaration that is accessible only to its lexical scope." nit, this is a dynamic scope, not a lexical scope. Lexical relates to syntax, these are related to the dynamic construction of tasks. This is closer to how name lookup works in elisp for example.
The Sendable proposal got accepted, so all references to ConcurrentValue should be removed and be included. Detailed design has several references that are out of date.
Yes, yes. We need something to solve these sorts of problems, and this is going in the right direction.
Yes, I've used a lot of TLS and queue local sorts of storage mechanisms. This seems like natural progression aligning with the Swift Concurrency model in general.
Participated a bit in the review thread, read the proposal in depth.
I also don't understand the concern here. This is how properties work in Swift. If you haven't called something to set a value, the only thing you could possibly get would be the default. So I agree with the others: the property-based syntax works best. It's just unfortunate that it isn't a general solution so we could avoid the type-based syntax completely. But the need for setters and lack of stored properties in extensions ties Swift's hands in that case.
Do you mean "wrong" in the sense that the implementation doesn't/can't handle the concept of task locals with a limited lifetime and per-instance identity? Because to me, the example you gave looks perfectly reasonable and the behaviour you describe just as I would expect it: Instance variables have different values for each instance. The fact that with a TaskLocal that value might have different facets for each task it's accessed from doesn't influence that at all.
In fact, if TaskLocal in the example were just a simple wrapper that provides scoped rebinding (withValue), with no knowledge or relation to tasks besides in name, the result would still remain the same. Is that also wrong?
Imo the declaration syntax as proposed doesn't really flow well, and I'm sure I'd have to look it up quite a few times before remembering it. The proposal mentions SwiftUIs environment as prior art, but this isn't quite true because: "there is no need for implementing set/get with any actual logic".
Instead, a computed variable on TaskLocalValues needs to be implemented in a way that seemingly will be the exact same for each key, but will—confusingly—never get called, but just used as a means to get access to key-path syntax.
The only real benefit from this verbosity/ceremony that I can make out is this: It becomes easy to get a list of all possible task local keys using auto-complete. Is that expected to be a common thing to do?
Another drawback I see is the following: Surfacing the task local values at a point in your API where it makes semantic sense becomes harder. You can write a wrapper function returning the value, but that obscures the task-localness.
From the proposal:
Tasks already require the capability to "carry metadata with them," and that metadata used to implement both cancellation, deadlines as well as priority propagation for a Task and its child tasks. Specifically Task API's exhibiting similar behavior are: Task.currentPriority() , Task.currentDeadline() and Task.isCancelled() .
I think it would be valuable if these "common" task local values could be declared (implementation might special case these for performance of course) using the same mechanism used for other task locals, to serve as introduction and examples of the concept, as well as making their task-localness clear from more than documentation or semantics alone.
So, if it were at all possible, I would much prefer a property wrapper syntax as described by Becca. It greatly reduces the verbosity of declaring and reading TLVs, provides natural and obvious place for default values and looks (imo) much more approachable.
One last thing I'd like to mention is this extract from the proposal, which confuses me a bit:
It is not recommended to abuse task local storage as weird side channel between child and parent tasks–please avoid such temptations, and only use task local variables to share things like identifiers, settings affecting execution of child tasks and values similar to those.
Since the whole mechanism introduced here serves as a side channel between child and parent tasks, what criteria would identify any particular use of TLVs as "weird"? Are there any performance pitfalls that are vitally important to be aware of, which this statement might have its source in?
Or should this be taken to mean that the four use-cases described in the proposal and only those four use-cases are not "weird", and that for any use-case that is not among those four, one should not use this feature?
Other than syntax and that little bit though, I think the proposal is on a great course. In particular I really like the decision to elide a setter and only provide scoped rebinding, as it makes reasoning about and controlling the lifetime of TLVs very easy.
Thanks for all the comments. I'll go through them chronologically.
Terribly sorry that the proposal had some lingering references to outdated wordings from other proposals. I cleaned that all up now, without changing any of the proposed semantics so it's still the same proposal.
--
I gave @beccadax's proposal a deeper look and am convinced this can work well - thank you, Becca.
The use of a property wrapper has a few effects on the design that have me worried, but perhaps they are not severe enough has a number of pieces that seem risky to me, I'd like to get feedback on.
So declarations are pretty nice:
enum MyLibrary {
@TaskLocal
static var traceID: Int = 0
// @TaskLocal(additionalOptions: here)
// static var traceID: Int = 0
}
and we're able to express the options that we definitely need in the future of this feature, so I'm happy with that.
Minor nitpicks with this:
As we discussed before, it is possible to forget to make it static; though perhaps I'm over worrying here and one would quickly realize the mistake in practice. I'll answer to some questions about why I consider it "wrong" as someone asked about this upthread.
small limitation: property wrappers on top level are not allowed, but may be in the future; So they must be defined in some namespace enum. This is fine I think.
So far it's all very nice.
--
Use sites then look like this:
// bind
await MyLibrary.$traceID.boundTo(1111) { // OR withValue(_:)
// OR withValue(boundTo:)
// ...
}
Minor issue 1: To be honest that's pretty ugly to me... but that's just how property wrappers work like, and I suppose people have gotten used to this already?
At least forgetting the $ does result in a helpful message, so that's good:
Reading the value also has me worried, because it looks too innocent:
// read
let value: Int = MyLibrary.traceID // <1>
Issue 2: This isn't an O(1) operation. We make it look as if it is just a plain old static property access. While in reality it is much more costly: a thread local access plus the scanning of the declared bindings. This may often be quick, especially with the implemented optimizations, but the worst case can be pretty bad (if there are many tasks, and each task added some value, but not the one we're looking for). It still is only O(n) in terms of bound values in the nightmare case, and arguably that case should be pretty rare.
If we, and the core team, collectively think this is a good tradeoff to make things much easier to use, even if easier to abuse we could go with this design. It is able to implement all semantic needs we have for this feature.
Thanks! I'm glad to see people recognize patterns they had to implement manually in the past in this feature and how it'll help them
One more piece of exciting news -- we have a feature complete implementation of open telemetry, zipkin and jaeger tracing using these APIs and we'll be sharing those shortly. It's been an effort that @slashmo has been working on for a while now I'll leave posting details of that to him though, it's all very exciting!
Sadly is a reason this cannot be implemented using a collection (two even):
allocating a collection and growing it to support more values as they are being bound will cause allocations. Task locals are designed to be very low overhead and we cannot afford to introduce APIs which force allocations.
he second issue is type-safety of such signature
we can't easily express (K, K.V)... where the K is different for every parameter. So it would end up either not type-safe (a no-go), or hardcoding e.g. "5" versions of such API.
In practice I don't think this matters all that much, because instrumentation libraries most often just set one or two values that they propagate, i.e. like W3C Baggage or "MyTraceContext" etc. which then contains a lot of values. They are usually carried in one task-local though. We know this from practical implementations of Tracer types over the last year in GitHub - apple/swift-distributed-tracing: Instrumentation library for Swift server applications.
There is no API for this (on purpose), the same way that there isn't an API to get "all thread locals on this thread". Some values you may not be allowed to read because they are private, some other values you should never print yet they must be carried around etc.
Imagine an instrumentation which propagates "authorization key" on behalf of a user such that certain APIs accept the authorized call. This is set by a framework and used by the framework. Developers should not be able to reach in and grab that key to store it and later on set it again etc.
I don't have any specific plans to share about Apple platforms and OSLog, but basically if there was some "trace id" that OSLog would want to be aware of, that key would be defined in OSLog or elsewhere in the SDK and OSLog could pick it up.
Baggage keys have additional properties such as name overrides and might also gain "please don't print this one" (I implemented this as "access control" for baggage items, but we decided to re-introduce it later-on) which the logging system would be aware of. This means that Swift Log might want to move away from using it's Metadata and rely on task locals and baggage instead as the propagation mechanism.
Baggage does offer APIs for iterating over all values it contains. Task local values do not offer such API. Baggage will replace the "metadata set on a logger" pattern in swift-log, enabling automatic metadata propagation. Loggers will be able to log all baggage items which are okey to be printed.
There's some history there and the Logger.Metadata is basically a manual metadata propagation mechanism -- which we're replacing with task locals by an automated one. There will be some changes on that (logging) API in the future to fully embrace locals.
Right, thank you -- indeed our goal here is to have this just-work™ but remain type-safe and familiar to Swift developers
Indeed today on the server we've been restricted to passing context objects manually, and it's been pretty annoying and not swifty. This is the primary reason the distributed-tracing APIs have not yet hit 1.0 – we've been waiting for this feature to land, and soon all major libraries will adopt and provide support for tracing right out of the box
This is arguing for not doing default values on declaration site, however assuming we did not do default values there... I don't think we should provide any such [..., default: defaultValue] style API, because it is no less verbose than [...] ?? defaultValue.
Having that said, since we seem to be moving towards the property wrapper based approach, we easily have a way to declare default values there.
No, inheritance options must be a property of the keys. I.e. a value must not be inherited etc. This is a property of the specific key, and not of an use site.
The one weird-off case is forcing inheritance with a detached task. Which I still have mixed feelings about... It feels like we're missing a primitive like send work() which is similar to detach but does not cause us to wait like await does. It would then still be a child task, and the use of detach would be–rightfully–very very frowned upon (as it should).
How much protection is that though, really? I suspect it wouldn't offer much resistance to anybody determined to find out that key - after all, they know it's stored in one of the task-local linked-list entries, and the source and ABI is publicly available.
There may be other reasons not to offer that API, but this one doesn't seem convincing IMHO.
Terribly sorry about this; I the proposal indeed has been catching up on all the other proposals moving since months and got a bit messy. I did a cleanup pass through the text now, it hopefully does not refer to old wording anymore.
Right, I'm very excited about this aspect actually. We're in a good spot with structured concurrency as a core concept to pull of patterns which are impossible in other runtimes and languages (e.g. the way we're using using efficient allocation the locals, and are also in some cases to allocate even tasks in their parent's task allocator).
Correct, it is possible to read task locals from synchronous functions. It is among the bigger reasons we made the task accessible in those.
It seems I missed a few sentences which still claimed that these APIs only work from asynchronous code - fixed these now. Sorry about this, the proposal should now be consistent.
The current design assumes that reads may be performed from asynchronous or synchronous functions. And if no task is available in the synchronous function, we return the default value.
I will point out that currently the design assumes that writes must be done in an async function:
// initially proposed API
Task.withLocal(\.traceID, boundTo: 1111) {
...
}
// or, following style of Becca's proposed API:
await Lib.traceID.withValue(...) {
...
}
This is because there must be a task available to set a task local value after all. And unlike for reads, there isn't really a good alternative to do when no task is available other than silently do nothing, which I can see as getting very very confusing. For example, it would be very confusing if we allowed bindings in synchronous functions like this:
func log() {
print("value: \(Task.local(\.value))")
// print("value: \(Library.value)") // in property wrapper style
}
// NOT PROPOSED API; this showcases issues with such approach
func sync() {
/*await*/ Task.withLocal(\.value, boundTo: 1) {
log()
}
}
If we allowed such API it would be very confusing, because depending on if sync() was called inside a task it'd work as expected; but if it wasn't, we'd have to no-op and set nothing...
We can definitely discuss if this is just the way it is and relax the restriction on binding task locals even from synchronous contexts. At the risk if this confusing example becoming a reality.
Alternatively, (and which was originally the plan), it would be possible to perform write operations on the UnsafeCurrentTask -- inside a synchronous function if we are able to get a task, we know we can use it to set the value.
I'd love your feedback @Chris_Lattner3 on this aspect of the design: Shall we permit writes (binding values) also from synchronous functions even given the above confusing snippet? One could argue that that's just what one has to deal with when trying to use task specific APIs in a context where a task may not be available...?
As it seems that we're closing on towards using the property wrapper design for declaring keys, the default value aspect becomes rather natural:
@TaskLocal
var traceID: TraceID? = nil
// or
@TaskLocal
var traceID: TraceID = .empty
This feels pretty natural to me; and the complexity isn't as much in the default values (even in the initial proposed design), as it is in the "options" which we need to support for future extensions of this API (the "inheritance modes" etc).
Nice, thanks! I'll admit I was not familiar with the origin of the technique but have seen it in practice in such context types and was very happy that our structured concurrency guarantees make it all work in this context. I'll read up a bit there, perhaps some other tricks exist we could apply here.
Great question! Yes, that indeed is the plan in order to get better lookup performance for lookup performance sensitive keys.
The current implementation does not do this yet but indeed we'll be able to reserve a few words for such in-line storage. I don't know yet the fine details of this, but the general idea was to provide e.g. 3 (or some small n) "slots" that we'd utilize for this.
Then, we'd want to utilize this storage for "privileged keys" which we'd mark using options on the key types. If no privileged keys are present, we could use it for other keys as well.
This would allow us to always store keys which will be "very very often accessed" in the in-line storage, rather than having to perform the search for them. For example this could be used for mach vouchers or traceIDs which are accessed all the time, while other less-privileged keys would use the usual linked list lookup method.
Detaches + carry task locals anyway are sadly quite expensive. They mean we have to copy all values (and +1 reference count them) over to the new task.
There is no way we can implement this without copying sadly. Task local values are optimized for the typical use of setting some values, running child tasks which read them, and then tearing down all that memory. Even tasks themselves we believe we'll be able to allocate using this task local allocator -- this will give noticable performance gains to short-lived tasks, as we will not have to malloc at all for short lived tasks.
So... yes detaching and carrying values is quite heavy, we have to iterate through the task local bindings and copy over the first encounter of any key to the new task.
The verb in the write side is "bind", as in Task.withLocal(_:boundTo:) but I can see your point.
Given that many in this thread would prefer the property wrapper API, what do you think about those?
But it's a bit ad-hoc treatment of one specific property wrapper so not sure we want to do that...
It is not recommended to abuse task local storage as weird side channel between child and parent tasks–please avoid such temptations, and only use task local variables to share things like identifiers, settings affecting execution of child tasks and values similar to those.