-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Developers can await a Task with a timeout #27723
Comments
Similar code is already in corefx; but only used for running tests rather than exposed as api Task TimeoutAfter(this Task task, int millisecondsTimeout)
Task<TResult> TimeoutAfter<TResult>(this Task<TResult> task, int millisecondsTimeout) |
CC @stephentoub |
I need to use this for production code in #42585, so might as well champion it for API review. Will come up with a proposal shortly. |
namespace System.Threading.Tasks
{
public static class TaskTimeoutExtensions
{
// throws TaskCanceledException
public static Task WithCancellation(this Task task, CancellationToken cancellationToken) => throw null;
public static Task<TResult> WithCancellation<TResult>(this Task<TResult> task, CancellationToken cancellationToken) => throw null;
// throws TimeoutException
public static Task WithTimeout(this Task task, TimeSpan timeout) => throw null;
public static Task<TResult> WithTimeout<TResult>(this Task<TResult> task, TimeSpan timeout) => throw null;
public static Task WithTimeout(this Task task, int millisecondsTimeout) => throw null;
public static Task<TResult> WithTimeout<TResult>(this Task<TResult> task, int millisecondsTimeout) => throw null;
}
} I haven't included |
The comment about throwing |
I think there are several questions that need to be answered before considering a concrete API. One is key use cases. One of the reasons we added ConfigureAwait as we did was the idea that we might in the future add additional overloads for things exactly like cancellation and timeouts, e.g. await task.ConfigureAwait(false, cancellationToken); There are pros and cons to this, in particular on the plus side is we don't actually need to allocate a Task and are free to do whatever we can internally to make this as efficient as possible (and it arguably makes the semantics clearer, that it's specifically about the await operation rather than about the task itself). The primary con is it's not as composable: if you get back a Task, you can use it as you would any other task, in many other situations than you could the awaitable struct returned from ConfigureAwait. Regarding ValueTask, it's a middle ground: it gives us a bit more flexibility in how we might optimize the implementation, at the expense of usability; if we expect the overwhelming use case to be There's also the question of having both timeouts and cancellation tokens that you want to use together, i.e. should these really be separately named methods, one for cancellation and one for timeout, or should a single API (maybe with overloads) let you utilize both at the same time, ala Task.Delay(int, cancellationToken). |
I like the |
What is the policy with respect to specifying the timeout as Has it been decided on how new APIs with a timeout should be designed? My opinion: Something like So maybe the |
Me, too. I would, though, like to hear from folks if there are compelling use cases for the WithTimeout/Cancellation methods where the result isn't immediately awaited. If there are meaningful such situations, that could move us in that direction, or possibly we could do both approaches (they're not mutually exclusive). |
@bartonjs, have we drawn any lines in the sand here? |
It should be fairly trivial to write such extension methods when an equivalent |
That's a good point, it's a one-liner. It'd have a bit more overhead, but unlikely to be prohibitive, especially since it'd almost certainly be less than existing solutions folks use today. |
We might still need to expose those extension methods for the benefit of languages like F# whose async implementation doesn't have first-class support for C#-style awaitables. |
It would probably still be worth the effort to implement the Task returning extensions without the additional exception throwing involved from async/await. |
Have you seen requests for this functionality from folks using F#?
It's not clear to me whether there's a real need for such methods for anything other than directly awaiting. Hence my question about use cases. But if there is, even if these did avoid one throw, it's likely only one of many, so I'm not sure it would make a meaningful difference. I'd be interested in learning otherwise. |
If you mean |
Example var probeCancellation = cancellation.WhenCancelled();
var probeTask = prober.Probe(SiloAddress, diagnosticProbeNumber);
var task = await Task.WhenAny(stopping, probeCancellation, probeTask); These implementations involve additional exception throwing/catching because they are built using async/await, but it would be possible to implement them more efficiently so that no exceptions are thrown inside the implementation (only in the consuming code if it uses |
I'm not following. Searching the repo, every use of those I see is directly awaited. |
Yes, that's the common case and I was just providing examples for the current situation that's suboptimal. I'm also in favour of the |
Oh, yes, totally agree. Implementing them in terms of ConfigureAwait is just an easy way to get that functionality if such methods aren't exposed. |
That's one of the reasons for not choosing TimeSpan, yes 😄. Especially if it's an overload of an existing method, or would be the only member on a type that uses TimeSpan while everything else uses int.
In 4.0? Yep. After 4.5? A little more tricky, since there's some asymmetry between synchronous methods (TimeSpan parameter) and asynchronous methods (CancellationToken can already represent this... how do we reconcile? (sad answer: we do a lot of metaphorical beard-scratching and harrumphing and call it a day with no answer)). Since this is effectively bridging the two, it seems like
Yep. But, as you can see, the sand is somewhat dry. |
It would be great if both timeouts and deadlines could be supported with the same technique. Deadlines being a core part of gRPC, and others. |
Thinking about deadlines is interesting. But I'd rather see a single helper added somewhere, e.g. along the lines of: public static class Timeout
{
public static TimeSpan FromDeadline(DateTimeOffset deadline)
{
TimeSpan timeout = deadline - DateTime.UtcNow;
return timeout < TimeSpan.Zero ? TimeSpan.Zero : timeout;
}
...
} rather than add yet another set of overloads to every method throughout the system that takes a timeout. That would be a separate proposal. |
I'd go with weaker wording than |
Let's keep such a proposal separate. I don't think it should impact this. |
TIL IAsyncEnumerable already supports this, I suppose we should probably use that API as a template for any Task/ValueTask proposal? |
Sort of: it has that API, but it does something different. It's there because the As such, it's then different from what's being discussed in this issue: it's not the equivalent of using the proposed |
Here's what an API for cancellable task awaitables could look like (wlog for the case of namespace System.Runtime.CompilerServices
{
public readonly struct ConfiguredCancelableTaskAwaitable<TResult>
{
public ConfiguredCancelableTaskAwaitable<TResult>.ConfiguredCancelableTaskAwaiter GetAwaiter();
public readonly struct ConfiguredCancelableTaskAwaiter : ICriticalNotifyCompletion
{
public bool IsCompleted { get; }
public TResult GetResult();
public void OnCompleted(Action continuation);
public void UnsafeOnCompleted(Action continuation);
}
}
}
namespace System.Threading.Tasks
{
public class Task<T>
{
public ConfiguredCancelableTaskAwaitable<TResult> ConfigureAwait(bool continueOnCapturedContext, CancellationToken cancellationToken);
}
} @stephentoub suggested we incorporate the AwaitBehavior enum proposal into this design. It should be possible to embed it into the existing namespace System.Threading.Tasks
{
[Flags]
public enum ConfigureAwaitBehavior
{
NoCapturedContext = 0x1, // equivalent to ConfigureAwait(false)
NoThrow = 0x2, // when set, no exceptions will be thrown for Faulted/Canceled
Asynchronous = 0x4, // force the continuation to be asynchronous
...
}
public class Task<T>
{
public ConfiguredTaskAwaitable<TResult> ConfigureAwait(ConfigureAwaitBehavior configureAwaitBehavior);
public ConfiguredCancelableTaskAwaitable<TResult> ConfigureAwait(ConfigureAwaitBehavior configureAwaitBehavior, CancellationToken cancellationToken);
}
} If we are in agreement, I can create a new issue containing an API proposal based on the above sketch, and we can probably close this issue and #22144. |
This should be prototyped. In particular I'm a little concerned about the extra logic this would likely add to ConfiguredTaskAwaiter's GetResult method, which should generally have its fast path inlined and be really lightweight, but with this we could unexpectedly find GetResult is no longer inlined (which would yield a relatively high overhead increase for synchronously completed operations) or that additional branches are inlined into every call site. We may want to make this overload return the ConfiguredCancelableTaskAwaitable instead; it's already going to be meatier in support of cancellation. (This doesn't need to block further progress/review, just that we need to be open to some tweaks like that, and definitely need to do perf testing along the way.) cc: @benaadams |
Good point. Sounds like we need a better name for |
Spent a bit of time today trying to prototype a |
Task.WithTimeout(TimeSpan)
method
To give a more concrete example of the above, adding a So I can really only see two possible paths here. Either
@stephentoub thoughts? |
I don't have a strong preference. I'd be fine updating the analyzer to simply ignore ConfigureAwait; it's logically part of the whole async/await system that the analyzer is about as well, so I don't feel badly about special-casing the method.
If we go that direction, we'd need WithCancellation to return a Task rather than some other awaitable. This needs to be composable with other options, like a ConfigureAwait that doesn't capture SynchronizationContext or that lets you await without throwing an exception. |
Eirik, let me know if you decide to modify CA2016's behavior to exclude ConfigureAwait. I wrote it and I'll be happy to help. |
A very popular StackOverflow Q&A asks how to await a
Task
with a timeout. A simpleTask.WithTimeout(TimeSpan)
method would alleviate a lot of time developers spend looking for how to properly do this, and avoid doing it wrong (e.g. leaving timers for finalization).See how we offer a
WithTimeout
extension method from vs-threading.The text was updated successfully, but these errors were encountered: