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 via the forum messaging feature. When contacting the review manager directly, please keep the proposal link at the top of the message.
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?
I can think of a simpler to use API. It could be a facade around the API suggested. Or maybe even the only API available, if there are no use cases that require lower-level primitives:
await withTaskPriorityEscalator { escalator in
await withCheckedContinuation { cc in
Task(escalator: escalator) { cc.resume() }
}
}
Hm, I don't think this really helps in unstructured situations, we're definitely seeing cases where "completely different task now has to escalate task (X)", especially when building cross process RPC systems, that's all they do for escalating work when other work is detected to need to escalate something "else" without actually waiting for it, we just need to "push" on it to get the work done quicker, and enqueue our work after it.
Effectively it comes down to a loop like this:
// SIMPLIFIED
Task {
for message in messages { // messages can have different targets
if message.escalation {
let targetTask = inFlight[message.target] // only if we have to escalate, per priority etc; there's quite some accounting to do here
Task.escalatePriority(of: targetTask, to: message.priority)
}
inFlight[message.target] = Task { deliver(message, to: message.target) }
}
}
}
So I do think we still want to offer the unstructured escalate API.
Mini update: this feature is now in nightly toolchains so you can give it a try: Swift.org - Install Swift -- select a "development snapshot" and you can try it out.
The feature is not gated behind any flag, but of course its inclusion in a real release is pending the Swift evolution review outcome. Thank you for all the input in advance!
Overall +1 on the proposal. I have personally ran into this limitation numerous times and had to write code that either used a continuation or an unstructured task which broke priority propagation. I'm glad we are exposing those tools finally.
Can we please use proper generic parameter names like Return and Failure here. This shows up in both auto completion and documentation.
Base/current priority
Task has two priority properties right now: basePriority and currentPriority. We have never clarified the semantics of those in a proposal and IMO it makes sense to add this here and explain which of the two is used and changed with the new APIs.
Example
Importantly, these two APIs seem to return potentially unexpected values right now.
@main
struct Foo {
static func main() async throws {
print(TaskPriority.high)
print(TaskPriority.medium)
print(TaskPriority.low)
print("Task: current:\(Task.currentPriority) base:\(Task.basePriority!)")
await withTaskGroup(of: Void.self) { group in
group.addTask(priority: .high) {
print("High child task: current:\(Task.currentPriority) base:\(Task.basePriority!)")
}
group.addTask(priority: .medium) {
print("Medium child task: current:\(Task.currentPriority) base:\(Task.basePriority!)")
}
group.addTask(priority: .low) {
print("Low child task: current:\(Task.currentPriority) base:\(Task.basePriority!)")
}
}
let task = Task(priority: .high) {
print("High unstructured task: current:\(Task.currentPriority) base:\(Task.basePriority!)")
}
await task.value
}
}
The above code prints the following
TaskPriority(rawValue: 25)
TaskPriority(rawValue: 21)
TaskPriority(rawValue: 17)
Task: current:TaskPriority(rawValue: 21) base:TaskPriority(rawValue: 21)
High child task: current:TaskPriority(rawValue: 21) base:TaskPriority(rawValue: 25)
Medium child task: current:TaskPriority(rawValue: 21) base:TaskPriority(rawValue: 21)
Low child task: current:TaskPriority(rawValue: 21) base:TaskPriority(rawValue: 17)
High unstructured task: current:TaskPriority(rawValue: 25) base:TaskPriority(rawValue: 25)
The child tasks can have a current priority that is lower than the base priority. Maybe I am just not understanding correctly how these values are derived.
+1 for the proposal. This is solving two real-world problems we’ve had in the development of SourceKit-LSP and needed to write somewhat hacky workarounds for (1, 2)
Conceptually +1 on the proposal, I gave it a decent read and have worked extensively with implementing APIs that use similar mechanisms. The one concern I have here is that: aren't we adding back the pyramid of dooms? So we got rid of those to add in async/await but getting them back with withTaskCancellationHandler, withTaskGroup, withCheckedContinuation and now withTaskPriorityEscalationHandler. It just seems like a lot... and some code will have tons of these nesting with odd consequences to the surrounding code because of that.
I like the functionality of being able to manipulate the priorities and do it safely, I'm just not thrilled at yet another closure to consider in the stack of things.
Yeah I feel you on that one... the structured nature of these forces us into with..., and I kinda wish we did some Python-style context manager so we'd with something to fight back on it all collectively, but right now there's no real alternative. (Without the with... { } these APIs would all have to be unsafe (bad) or cause heap allocations (bad bad)).
The alternatives considered has the withCheckedContinuation2 which would jam all the "onSomething:" into one API, so it'd be one with block with many onCancel:onPriorityEscalated: however as we looked into it the binding this to specifically continuation turned out to be too limiting...
So I agree, but I think we have a holistic problem that we need to address here with all the with... methods, and that'd then be a bigger task but also lots of more code would benefit from it.
Is it true that static methods are more 'hidden'? Particularly on Task, where there are a number of key APIs that are spelled idiomatically by typing Task (including, of course, Task.init and Task.detached). Following this logic to its endpoint, we'd be better off spelling these something like Task.zzzEscalatePriority and so on to really hide it...
"Salting" APIs with circumlocutions like this hasn't really been our practice and feels...unseemly. These are semantically operations on specific task instances and we have standard API naming practices for these—namely, instance methods.
As with all APIs, their proper use ought to be properly documented, and if they are a good feature but possibly an attractive nuisance, we should do everything we can to make the right thing easier to reach for. But I'm struggling to see a reason why APIs shouldn't align with our standard practices here.
To be clear; I am definitely in favor of this proposal being a tool in the toolbox - it just seems like the need for a more generalized system for managing effects or other such flavors applied to async things is ever increasing.
The only reason is autocompletion really, so not as much "hidden" as aiming to prevent stumbling into task.escalate(...) and go "uuu, tempting, let's do that" when I was about to write await task.value which will do the escalation in a much better way (with dependency tracking "who is waiting for this").
It's not a strong take though, and we can move to instance methods if we'd prefer that.
So while this may cause some suboptimal behavior it would not prevent the dependency tracking done by the await... So it's not the end of the world if someone did that. We'll highlight this in documentation. People can always do the wrong thing anyway if they're determined enough
For the sake of completeness, there was a discussion about this in the pitch:
Mentioned this in the pitch as well, but I am not sure if there was an opinion on that. Is there a reason, why isolation is the last parameter? Or other way around, is there a reason why it should not be the first?
I am asking, because the trailing closure syntax sugar will not work with how this is spelled, if you want to pass along the isolation. I think in standard library both approaches exist: withTaskGroup has the isolation before the body closure, but withTaskCancellationHandler has it after.
Comparison
withTaskPriorityEscalationHandler(operation: {
}, onPriorityEscalated: { _ in
}, isolation: someIsolation)
vs
withTaskPriorityEscalationHandler2(isolation: someIsolation) {
} onPriorityEscalated: { _ in
}
Should the callback take both the current and new priority? The runtime has this information at hand, and the callback might otherwise have to do something inefficient to get it.
Yeah, we could add the current one; you're right that there's no great way to get it otherwise because the Task.currentPriority is of whatever task the callback is invoked on and not of the being escalated task.
So like this:
onPriorityEscalated: { old, newPriority in }
I didn't see much use for the current but maybe it can be useful to know where from to remove the task as it is moved to the "new bucket" or something like that
I’m experimenting with a TaskLimiter which constrains the number of tasks entering a critical region. Waiting tasks are kept in a container with buckets for each priority. On priority escalation the continuation in the container has to be moved to the proper bucket. For this it is really helpful to know both the old and new priorities.
