Skip to content
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

Closed
AArnott opened this issue Oct 24, 2018 · 35 comments
Closed

Developers can await a Task with a timeout #27723

AArnott opened this issue Oct 24, 2018 · 35 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks Bottom Up Work Not part of a theme, epic, or user story Cost:M Work that requires one engineer up to 2 weeks Priority:3 Work that is nice to have Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@AArnott
Copy link
Contributor

AArnott commented Oct 24, 2018

A very popular StackOverflow Q&A asks how to await a Task with a timeout. A simple Task.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.

@benaadams
Copy link
Member

benaadams commented Oct 24, 2018

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)

https://github.com/dotnet/corefx/blob/407c09d5840384fbd24bbd5b8869ef13be71de63/src/Common/tests/System/Threading/Tasks/TaskTimeoutExtensions.cs#L28-L43

@tarekgh
Copy link
Member

tarekgh commented Jan 17, 2019

CC @stephentoub

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Feb 25, 2020
@stephentoub stephentoub modified the milestones: 5.0, Future Feb 25, 2020
@eiriktsarpalis eiriktsarpalis self-assigned this Sep 24, 2020
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 6.0.0 Sep 24, 2020
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Sep 24, 2020

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.

@eiriktsarpalis
Copy link
Member

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 ValueTask overloads, since it should be straightforward to convert them to task instances.

@pentp
Copy link
Contributor

pentp commented Sep 24, 2020

The comment about throwing TaskCanceledException means that the Task will be marked as canceled, not faulted, right?

@stephentoub
Copy link
Member

stephentoub commented Sep 24, 2020

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 await task.WithWhatever, then ValueTask could be the right choice instead of (not in addition to) Task. If we expect it to be something else, then Task is the right answer.

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).

@eiriktsarpalis
Copy link
Member

I like the ConfigureAwait overload approach, since it more clearly communicates that it is not the underlying task that is being cancelled, but it only concerns this particular awaiter.

@GSPP
Copy link

GSPP commented Sep 25, 2020

What is the policy with respect to specifying the timeout as int millisecondsTimeout or as a TimeSpan? It seems to me that if .NET could be done "greenfield" there only should be TimeSpan and never an int. But there's an argument for keeping consistency with existing APIs.

Has it been decided on how new APIs with a timeout should be designed?

My opinion: Something like await Task.Delay(100) is an anti-pattern. All code should use the TimeSpan form. New APIs should only use TimeSpan because getting rid of that unfortunate timeout style is more important than keeping consistency.

So maybe the int millisecondsTimeout overloads should be removed from this proposal.

@stephentoub
Copy link
Member

I like the ConfigureAwait overload approach

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).

@stephentoub
Copy link
Member

What is the policy with respect to specifying the timeout as int millisecondsTimeout or as a TimeSpan?

@bartonjs, have we drawn any lines in the sand here?

@eiriktsarpalis
Copy link
Member

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.

It should be fairly trivial to write such extension methods when an equivalent ConfigureAwait overload is available.

@stephentoub
Copy link
Member

stephentoub commented Sep 25, 2020

It should be fairly trivial to write such extension methods when an equivalent ConfigureAwait overload is available.

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.

@eiriktsarpalis
Copy link
Member

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.

@pentp
Copy link
Contributor

pentp commented Sep 25, 2020

It would probably still be worth the effort to implement the Task returning extensions without the additional exception throwing involved from async/await.

@stephentoub
Copy link
Member

stephentoub commented Sep 25, 2020

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.

Have you seen requests for this functionality from folks using F#?

It would probably still be worth the effort to implement the Task returning extensions without the additional exception throwing involved from async/await.

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.

@eiriktsarpalis
Copy link
Member

Have you seen requests for this functionality from folks using F#?

If you mean WithTimeout-like functionality, then yes, it's pretty common to knock together such methods in "Utils" modules.

@pentp
Copy link
Contributor

pentp commented Sep 25, 2020

Example WithTimeout, WithCancellation.
And a third variation that is used to reduce the number of exceptions thrown - WhenCancelled, related API proposal #37505, example use case:

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 await).

@stephentoub
Copy link
Member

Example WithTimeout, WithCancellation

I'm not following. Searching the repo, every use of those I see is directly awaited.

@pentp
Copy link
Contributor

pentp commented Sep 25, 2020

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 ConfigureAwait approach. But if we're going to implement Task returning variants also, then I would expect them to not throw exceptions internally.

@stephentoub
Copy link
Member

But if we're going to implement Task returning variants also, then I would expect them to not throw exceptions internally.

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.

@bartonjs
Copy link
Member

What is the policy with respect to specifying the timeout as int millisecondsTimeout or as a TimeSpan?

  • DO prefer using TimeSpan to represent timeout time.

But there's an argument for keeping consistency with existing APIs.

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.

Has it been decided on how new APIs with a timeout should be designed?

In 4.0? Yep. TimeSpan, unless there's a good reason not to. (Such as the quantum of time from an underlying API call is in minutes, and no one would expect that TimeSpan.FromSeconds(2) is either 0 or 1 minute).

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 TimeSpan is the clear winner.

@bartonjs, have we drawn any lines in the sand here?

Yep. But, as you can see, the sand is somewhat dry.

@fowl2
Copy link

fowl2 commented Sep 26, 2020

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.

@stephentoub
Copy link
Member

stephentoub commented Sep 26, 2020

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.

@benaadams
Copy link
Member

I'd go with weaker wording than Deadline as it's going to start drifting between getting it and using it; also the triggering won't be exact (15 ms range?).

@stephentoub
Copy link
Member

Let's keep such a proposal separate. I don't think it should impact this.

@eiriktsarpalis
Copy link
Member

TIL IAsyncEnumerable already supports this, I suppose we should probably use that API as a template for any Task/ValueTask proposal?

@stephentoub
Copy link
Member

TIL IAsyncEnumerable already supports this

Sort of: it has that API, but it does something different.

It's there because the GetAsyncEnumerator interface accepts a CancellationToken, but if you're using the language await foreach support, you're not explicitly calling GetAsyncEnumerator, rather code generated for you is doing so... so this extension method exists to let you pass a token through. And while it could have just been called ConfigureAwait, it doesn't because it's not just about adding ConfigureAwait onto awaits, but rather what arguments are passed to GetAsyncEnumerator.

As such, it's then different from what's being discussed in this issue: it's not the equivalent of using the proposed WithCancellation on each await. If, for example, an iterator ignored its [EnumeratorCancellation] argument, the CancellationToken supplied to the WithCancellation would be a nop. In contrast, the one in this proposal would actually cancel the await even as the underlying operation represented by the task may still be in flight.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 15, 2020

Here's what an API for cancellable task awaitables could look like (wlog for the case of Task<T>):

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 ConfiguredTaskAwaitable struct without any breaking changes, so assuming we go ahead with it we should also include the following APIs:

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.

@stephentoub
Copy link
Member

stephentoub commented Oct 15, 2020

public ConfiguredTaskAwaitable ConfigureAwait(ConfigureAwaitBehavior configureAwaitBehavior);

If we are in agreement

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

@eiriktsarpalis
Copy link
Member

Good point. Sounds like we need a better name for ConfiguredCancelableTaskAwaitable?

@eiriktsarpalis
Copy link
Member

Spent a bit of time today trying to prototype a ConfigureAwait(bool, CancellationToken); method. A side-effect of making this addition was that existing code using the original ConfigureAwait(bool) overload would now trigger the CA2016 analyzer warning. So we would probably need to either simultaneously update the analyzer to ignore this particular pattern or consider a different design for task cancellation altogether.

@jeffhandley jeffhandley self-assigned this Jan 14, 2021
@jeffhandley jeffhandley added the Priority:3 Work that is nice to have label Jan 14, 2021
@eiriktsarpalis eiriktsarpalis added Cost:M Work that requires one engineer up to 2 weeks Bottom Up Work Not part of a theme, epic, or user story User Story A single user-facing feature. Can be grouped under an epic. Team:Libraries labels Jan 14, 2021
@danmoseley danmoseley changed the title Add Task.WithTimeout(TimeSpan) method Developers can await a Task with a timeout Jan 20, 2021
@eiriktsarpalis
Copy link
Member

To give a more concrete example of the above, adding a Task.ConfigureAwait(bool, CancellationToken) overload results in the CA2016 warning being emitted for all task.ConfigureAwait(false); calls.

image

So I can really only see two possible paths here. Either

  1. Update the analyzer so that ConfigureAwait is excluded before we make the change, or
  2. Adopt a different name altogether, for example WithCancellation.

@stephentoub thoughts?

@stephentoub
Copy link
Member

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.

Adopt a different name altogether, for example WithCancellation

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.

@carlossanlop
Copy link
Member

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.

@eiriktsarpalis
Copy link
Member

I've created issue #47525 which combines the proposals of #27723 and #22144. Closing this one.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks Bottom Up Work Not part of a theme, epic, or user story Cost:M Work that requires one engineer up to 2 weeks Priority:3 Work that is nice to have Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

No branches or pull requests