I suppose the issue is that there's no guarantee of consistent behavior between Clock implementations, since the protocol doesn't say there has to be. Nothing defines what nil means. Looking at the implementation, it appears the executor actually controls what tolerance does in executor.enqueue(_:at:tolerance:clock:), so really it appears to be up to the executor to say what nil means. That means it's already platform specific, and could be customized by custom executors (though it seems the scheduling executor API is SPI?).
I made a similar comment during the previous review. A nil tolerance is implementation defined. I think it would be reasonable for this to be customisable (environment variable? global setting?) to mean no tolerance for server applications and have the current behaviour for client apps.
[edit] I think the meaning of tolerance:nil would need to be a process wide setting, because otherwise a library using the default tolerance would have the wrong behaviour on one or other platform.[/edit]
That seems like a separate (but still important) issue to what’s in this proposal, which is just continuing with the existing behaviour.
Could the deadline task local be a Deadline type, which erases the clock and instant, and allows accessing a UTC instant for IPC/RPC use cases? If the overhead of working out the earliest deadline is too much when calling withDeadline, this type could contain all the deadlines, then calculate the earliest if it’s requested.
extension Task {
var deadline: Deadline { ... }
}
struct Deadline {
function time<C: Clock>(clock: C) -> C.Instant? { … }
}
The way this is handled in Go is by parameterising cancelation with an Error cause - see WithDeadlineCause / WithCancelCause
I think the equivalent in Swift would be to change CancellationError.reason to var cause: (any Error)?. The default for withDeadline would be a new DeadlineError.
1 Like
There is no UTC interpretation for a manually-stepped testing clock.
There isn’t really a UTC interpretation for an instant of SuspendingClock, either. Like, there is if the system definitely doesn’t suspend between now and then, but if that’s what you wanted, you would’ve used ContinuousClock.
Can you outline your security concerns here? I can see DDoS vector where an attacker can take advantage of fixed time intervals to spike CPU usage artificially, but I'd think a
nildefault actually works against that. By allowing the systems to perform their own scheduling, they can more intelligently track work scheduled at specific times and spread it out. What default would you like to see?
Yes, except that the DOS doesn't need to be distributed. For example, if you have a situation where all timers that fire within a 5 second interval get coalesced into one precise point in time. This allows an attacker 5 seconds which massively amplifies the work that will trigger at one precise point in time. This is bad. It goes from latency problems all the way to a DOS.
I think your claim is too broad here. Apple spent a decade getting app developers away from precise timer callbacks in order to better optimize power usage and overall system performance, especially on mobile devices. Coalescing a lot of work into narrow periods is exactly what you want when you're concerned with balancing maximum CPU performance with minimum battery burn.
I believe this is too simplistic with the current CPUs. In fact, it might even be counter productive. I don't know the precise figures but I think these days it's probably close to true that one E core is essentially always running. The others (and most importantly the P cores) may be asleep. So by allowing too many timers to be coalesced together, you may even change have to wake up other E or possibly even a P core to deal with the work.
I generally agree, although I’m not sure how it could ever not be up to the clock. We cannot guarantee precise timer behavior in any event.
That's fine. But the default should be non-coalesced. The principle of the least surprise is important. I don't mind if we change the clocks to default to not coalescing or if we default to not allowing the clocks to coalesce. Again, this is not about precision, it's about not coalescing timers.
And this is extra important for timeouts/deadlines because they tend to be longer and the current implementation makes the coalescing windows larger the longer your timer interval is.
I believe this is the formula on Linux and whilst this isn't published officially, let's for now assume it's the same on Apple platforms. This is 10% of the sleep!!! So if you have a 5 minutes timeout (not unreasonable), then we're gonna coalesce everything within 30 seconds into one instant of time. That will break things if you run production services at a certain scale.
4 Likes
Happy to see there's no .deadlineExpired error to be thrown anymore, so no one will expect that error to be thrown unconditionally on deadline expiration. I find the new API a lot less surprising (in a good way).
Still, for the semantics I'm most likely to want as an app developer, I'm not sure withDeadline works. Even though the proposal says this:
Users who wish to adjust behaviors can use the task cancellation shields and/or task cancellation handlers to alter the behavior of the return values. These in conjunction with manual processing of do/catch clauses can compose to complex behaviors needed for many specialized scenarios.
I don't see an easy way to compose these functions to get the behavior I would want from a deadline!
Most of the time, what I'd want to achieve is:
1. Do X for up to N seconds.
2. If the deadline is exceeded, do Y instead.
I understand that in order to achieve (1) I need X to gracefully handle task cancellation. Fine. But I'm not sure what I'd need to do in order to achieve (2) if I want (2) to happen no matter what X does after it's deadlined (whether it returns a value, throws...).
This should be a common need, for example whenever X happens to handle cancellation by inserting guard !Task.isCancelled else { return } (an extremely common pattern!).
At first I thought about using withTaskCancellationHandler, but alas, this doesn't work:
try withDeadline(clock.now.advanced(by: .seconds(5)) {
try await withTaskCancellationHandler {
try await doThing()
} onCancel: {
switch cancellationError.reason { // <-- Oops, no cancellationError here.
case .deadlineExpired:
deadlineExceeded = true // <-- Propagate somehow...
default:
break
}
}
}
Because a task cancellation handler doesn't expose the underlying CancellationError (it had no reason to, until now).
So here goes my first question: should withTaskCancellationHandler's have a new overload where onCancel is @Sendable (CancellationError) -> Void instead of @Sendable () -> Void? It should be possible to inspect the cancellation reason in a cancellation handler!
Regardless of the above, there are other issues with using a cancellation handler to reconstruct this behavior (can't throw from onCancel, toggling a captured bool isn't concurrency safe...).
So, on to my next option. Call try Task.checkCancellation at the end and catch the error:
// ⚠️ This code is buggy!
try withDeadline(clock.now.advanced(by: .seconds(5)) {
try await doThing()
do {
try Task.checkCancellation()
} catch let cancellationError = error as? CancellationError {
switch cancellationError.reason {
case .deadlineExpired:
throw MyError.deadlineExceeded
default:
break
}
}
}
But this also doesn't work:
- If
doThing()throws after the deadline is exceeded, the deadline check never runs, which isn't the behavior I wanted. - The deadline could expire after
doThing()completes but beforetry Task.checkCancellationruns, resulting in a false positive[1].
All this begs the question: should withDeadline have a onDeadlineExceeded optional closure parameter?[2] Or, alternatively, could there be some other way of knowing if the deadline was exceeded regardless of what happens with the operation after the deadline?
I know this is inherently racy because the deadline could be exceeded close to the operation finishing, but it's better than manually inserting a check. ↩︎
This would run into similar constraints as
withTaskCancellationHandler'sonCancel: may happen synchronously with the closure, can'tthrowfrom it (which is what you'd want to do most of the time)... but it's better than nothing. ↩︎
1 Like
It is not. Server and client CPUs are wildly different in how they want to balance work for maximum efficiency. Servers really want to be as close to steady state as possible. Most clients want to wake up, do everything and then get back into a low-power state as much as possible.¹ For this reason, nil seems like the only sensible default, because we can plausibly define it to mean "whatever is best for the platform that we're running on"; any other default would necessarily be suboptimal on one or the other.
¹ It's clear enough why you want servers to be in steady-state; why do clients want to be bursty? Actual data tells the story, but the intuition is fairly easy to explain: waking up a CPU means providing baseline power to a bunch of different functional units, not all of which will actually be completely used by any given code that happens to be running. So long as you're awake, and during a warm-up/cool-down period on each side of the work, you're paying that overhead whether or not you are using it. Doing as much work as you can do in a continuous burst minimizes the amount of power you waste (and therefore also the heat you have to dissipate) on that baseline in aggregate.
Modern core (and un-core) designs are getting better and better at fine-grained power gating which has driven this baseline overhead down, but it's still not zero, nor will it be anytime soon, and it's still high enough that the basic principle of racing to idle on client is beneficial.
11 Likes
I agree that nil is the right default, and I believe that it will become less of a problem once we make the default executors customizable and adopt swift-platform-executors. The default executors should decide what nil means for their use case. However, advanced users might want to change the default executors for one that handles nil differently.
3 Likes
It’s probably more philosophically convincing to note that servers are also very bursty, and we just expect that to be handled in a totally different way. If you had a 90’s-style monolithic server handling a low enough volume of requests that the system could frequently idle, it would probably benefit from client-like power optimization. You’d need timers on the path of blocking client responses to be prioritized and delivered promptly, but that is a well-known problem for UI programmers and can be solved well. It’s just that people don’t generally write servers that way anymore, for good reason. Instead, we have load balancers that bring instances up and down dynamically in response to demand. Just letting that do its thing to make sure that instances are well-saturated is a more cost-effective use of programmer time than picking up pennies with scheduler tricks on underutilized instances, and that’s what drives the split here.
2 Likes
If nil is the right default or not depends on what it means. On most platforms we will want it to mean "no precision requirements, no coalescing" by default.
This is critical. And it is not what it does today and that's a problem.
We should not ship withDeadline with a nil default if it does the same thing as Task.sleep does. It's bad, it has caused outages, it is a security issue and I don't think it's valid to just pretend that we'll fix it at a future point in time. Let's fix it now, with this proposal.
Leaving bad behaviours be the default for an indeterminate amount of time will just cause debates about compatibility in the future.
2 Likes
Isn’t it more of an application default, rather than a platform default? Desktop applications on macOS, iOS, etc. should be using timer coalescing and loose tolerances whenever possible. But games often want timers with moderate-to-tight tolerances and no coalescing.
1 Like
I would expect an application that needs a specific tolerance to specify the tolerance it needs. nil communicates "do whatever the clock/platform thinks is best for general purposes".
2 Likes
Yeah I agree we have to make sure in this proposal that the nil behavior is well defined here and does the right for the platform thing, as @johannesweiss is arguing.
2 Likes
I would expect an application that needs a specific tolerance to specify the tolerance it needs. nil communicates "do whatever the clock/platform thinks is best for general purposes".
Are you arguing that these applications specify tolerance: .zero at every withDeadline { ... } callsite?
If so, two questions:
- Why are we prioritising potential power savings over potential security issues by default?
- What should libraries do?
@ksluder makes a good argument: I can sympathise with that being an application-specific build time setting (or some magic global that can be set pre/at main-execution time). If there were a global build setting that lets you easily choose the default coalescing behaviour, this would all be much worse.
And to add on here, tolerance is defined as a Duration, so there's zero flexibility for callers to control behavior beyond hard limits. Aside from nil, which really just defers to the executor's default behavior, there's really no other control here at all. To solve this issue it would seem like we'd need an actual Tolerance type which explicitly allows for control over coalescing behavior or deferral to the OS or executor. Otherwise I think the only option right now is a custom executor, which can be difficult to work with, and that's if the executor even allows control of this property (the DispatchSerialExecutor does not).
2 Likes
If the goal were to harden against DoS attacks via this API, we would add exponential noise to the requested deadline rather than default to a tolerance of zero. I do not think that's what you actually want, however.
You want the default to be zero in some contexts, and that seems defensible; other contexts want it to be non-zero for well-founded reasons. nil is the best value available to mean something different in different contexts with this API, so it's the natural choice.
Use zero if they need a precise deadline. Use a non-zero value if they have a tight upper bound on how much variance they can accept. Allow clients to specify a tolerance to use if that's appropriate. Use nil otherwise to delegate to the clock/runtime to do whatever is most efficient on the platform on which they end up running.
7 Likes
Right. I also feel like the default tolerance shouldn't be inconsistent between different timer-related APIs. If there's a problem with the default behavior of sleep where nil tolerances are leading it to be too aggressive about coalescing timers and creating potentially unbounded CPU spikes, we should just fix that, not treat it as a tragic mistake that must not be repeated in other APIs.
8 Likes
If the goal were to harden against DoS attacks via this API, we would add exponential noise to the requested deadline rather than default to a tolerance of zero. I do not think that's what you actually want, however.
I mean, concretely what I normally do is (pseudo code) Task.sleep(amount + jitter, tolerance: .zero) and I think I wrote it elsewhere on this or a prior thread that ideally we'd ship it with jitter controls. But I can at least retrofit that easily.
You want the default to be zero in some contexts, and that seems defensible; other contexts want it to be non-zero for well-founded reasons.
nilis the best value available to mean something different in different contexts with this API, so it's the natural choice.
I need no coalescing but I don't have particular precision requirements. Especially not the huge amounts (10% if I read the code right) of coalescing that is currently done. That's unacceptable and we should only default to nil if nil does something safe. If nil in the normal executors meant tolerance: .milliseconds(1) I would be much less concerned.
Maybe we should take this as an opportunity and separate the 'allowable coalescing window' from 'precision requirements'. Currently, the only control we have is tolerance and the executor decides what the default is (and it chooses poorly, especially for deadlines which tend to be longer).
Use zero if they need a precise deadline. Use a non-zero value if they have a tight upper bound on how much variance they can accept. Allow clients to specify a tolerance to use if that's appropriate. Use nil otherwise to delegate to the clock/runtime to do whatever is most efficient on the platform on which they end up running.
Do we agree that with today's settings, they have to specify .zero (or something very small) in order to not massively facilitate DoS attacks and definitely introduce latency spikes?
My answer is that yes, I am asking for changes in pull requests that use Task.sleep to either specify tolerance: .zero or use our own Task.sleep variant which takes maxJitter and always sets tolerance: .zero.