-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Proposal: GetAwaiter for tuple of tasks #20166
Comments
We would want a formal API proposal before implementation. I think this is a useful api to have. The right place for them would be in https://github.com/dotnet/corefx/tree/master/src/System.Threading.Tasks/src/System/Threading/Tasks. |
@alexperovich Thanks. Would you have a past example of formal API proposal that we could use as template? |
Awaiting multiple asynchronous operations is a very common pattern in my project. It's a public-facing WebAPI portal to a large number of microservices where it's normal for a single incoming request to require making multiple backend HTTP requests to aggregate the data. We currently use some helper extension methods to return a |
This explains the whole workflow to add new APIs: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md |
Fyi, added |
What is needed to push along this proposal – a more formal write-up? |
@jnm2 the "assigned person" already replied a year ago: https://github.com/dotnet/corefx/issues/16010#issuecomment-278776513 |
ProposalThere are three parts which can be considered separately. 1. Simple awaitingExample: var (foo, bar) = await (GetFooAsync(), GetBarAsync()); Proposed API: namespace System.Threading.Tasks
{
public static class TaskTupleExtensions
{
public static TaskTupleExtensions.TupleTaskAwaiter<T1, T2> GetAwaiter<T1, T2>(this (Task<T1>, Task<T2>) tasks);
public struct TupleTaskAwaiter<T1, T2> : System.Runtime.CompilerServices.ICriticalNotifyCompletion
{
public bool IsCompleted { get; }
public TupleTaskAwaiter((Task<T1>, Task<T2>) tasks);
public (T1, T2) GetResult();
public void OnCompleted(System.Action continuation);
[System.Security.SecurityCritical]
public void UnsafeOnCompleted(System.Action continuation);
}
}
} 2. Configured awaitingExample: var (foo, bar) = await (GetFooAsync(), GetBarAsync()).ConfigureAwait(false); Proposed API: namespace System.Threading.Tasks
{
public static class TaskTupleExtensions
{
public static TaskTupleExtensions.TupleConfiguredTaskAwaitable<T1, T2> ConfigureAwait<T1, T2>(this (Task<T1>, Task<T2>) tasks, bool continueOnCapturedContext);
public struct TupleConfiguredTaskAwaitable<T1, T2>
{
public TupleConfiguredTaskAwaitable((Task<T1>, Task<T2>) tasks, bool continueOnCapturedContext);
public TaskTupleExtensions.TupleConfiguredTaskAwaitable<T1, T2>.Awaiter GetAwaiter();
public struct Awaiter : System.Runtime.CompilerServices.ICriticalNotifyCompletion
{
public bool IsCompleted { get; }
public Awaiter((Task<T1>, Task<T2>) tasks, bool continueOnCapturedContext);
public (T1, T2) GetResult();
public void OnCompleted(System.Action continuation);
[System.Security.SecurityCritical]
public void UnsafeOnCompleted(System.Action continuation);
}
}
}
} 3. Void-returning awaitingExample: await (DoFooAsync(), DoBarAsync());
await (DoFooAsync(), DoBarAsync()).ConfigureAwait(false); Proposed API: namespace System.Threading.Tasks
{
public static class TaskTupleExtensions
{
public static System.Runtime.CompilerServices.TaskAwaiter GetAwaiter(this (Task, Task) tasks);
public static System.Runtime.CompilerServices.ConfiguredTaskAwaitable ConfigureAwait(this (Task, Task) tasks, bool continueOnCapturedContext);
}
} Higher aritiesThe above can be trivially generalized to any higher arity. Ten is tentatively suggested. I don’t recall seeing arities greater than four in practice. Eight would be weird because the return type would be If you actually need to parallelize five async operations, there’s nothing that performs better and is easier to write than the following: Example: var (
dependency1,
dependency2,
dependency3,
dependency4,
dependency5
) = await (
GetDependency1Async(…),
GetDependency2Async(…),
GetDependency3Async(…),
GetDependency4Async(…),
GetDependency5Async(…)
).ConfigureAwait(false); Degenerate arityThe unary case could probably be left out for now since C# has no syntax for 1-tuples, but here it is for completeness: Example: var foo = await ValueTuple.Create(GetFooAsync());
var bar = await ValueTuple.Create(GeBarAsync()).ConfigureAwait(false);
await ValueTuple.Create(DoFooAsync());
await ValueTuple.Create(DoBarAsync()).ConfigureAwait(false); Proposed API: namespace System.Threading.Tasks
{
public static class TaskTupleExtensions
{
public static System.Runtime.CompilerServices.TaskAwaiter GetAwaiter(this System.ValueTuple<Task> tasks);
public static System.Runtime.CompilerServices.TaskAwaiter<T1> GetAwaiter<T1>(this System.ValueTuple<Task<T1>> tasks);
public static System.Runtime.CompilerServices.ConfiguredTaskAwaitable ConfigureAwait(this System.ValueTuple<Task> tasks, bool continueOnCapturedContext);
public static System.Runtime.CompilerServices.ConfiguredTaskAwaitable<T1> ConfigureAwait<T1>(this System.ValueTuple<Task<T1>> tasks, bool continueOnCapturedContext);
}
} QuestionsShould Entire API sethttps://gist.github.com/jnm2/47a6a014831615bdb7b64a75f09a5799 @alexperovich, is there anything else you need? /cc @buvinghausen, fyi |
@karelz Thanks for clearing that up. Is there anything I've missed? |
@kouvel will do first-pass of the API review as area expert & owner. |
@karelz I realize I'm supposed to own this area but I am no expert in this area. The proposal looks fine to me, CC @stephentoub |
ufcpp has suggest this public static System.Runtime.CompilerServices.TaskAwaiter<(T1, T2)> GetAwaiter<T1, T2>(this (Task<T1>, Task<T2>) tasks)
{
return Task.Run(async() => (await tasks.Item1,await tasks.Item2)).GetAwaiter();
} Work like charmed public static async Task Test()
{
var (x,y) = await (Task.Run(() => 0),Task.Run(() => 0));
} Just need to put it in BCL for tuple |
But then again would it possible to have a mixed mode? I mean var (x,y) = await (0,TaskInt); // became (int,int); Which then we need permutation of all task/nontask tuple combination? |
@Thaina Yes, this issue is about moving the suggested methods into the BCL. I don't see a benefit to the so-called mixed mode. The syntax would be contradictory to what is happening since you aren't awaiting both things, and it would require an exponential number of overloads per arity rather than a single overload per parity. It's rare but not unheard-of to await a |
@jnm2 That's the reason I was propose dotnet/corefx#1454 and prefer dotnet/csharplang#1454 (comment) than this issue |
@Thaina dotnet/csharplang#1454 has one big issue with it that I'm not sure I like: if you use shapes, there's no |
@jnm2 I don't think that would be a problem. Because we don't really need Or is it another reason you would need |
|
@Thaina Let's move the discussion to the csharplang issue, since this one is about BCL methods for C# 7. |
If this was a discussion about I would expect a tuple of awaitables to be itself awaitable. Since this is pattern-based language feature, I don't see a way to add it as a library without a combinatorial explosion. The standalone library appears to be a fine placeholder in the absence of language support. |
I discussed this with @jnm2 separately. It sounds like the implementation of the proposed method behaves the way I would expect for
However, given the ability to implement the requested functionality in a separate library with apparently no negative impact on performance or usability, I don't see a pressing concern that would need to move this issue forward prior to reaching an agreement for if/how the language would want to implement a similar feature within the compiler. The most important concern for me is ensuring |
I'd be fine with that. |
A nuget package already exists (TaskTupleAwaiter). I'm using it in practically every project I have that does |
Interesting, looking at nuget I am seeing the following:
which looks there is some demand on that. @stephentoub considering the demand, I think this will be better to be in the core. if you don't have a strong feeling against, I'll proceed marking this ready for review. |
That's pretty good for a NuGet package that isn't advertised. I haven't used it myself because I copy my gist into so many projects, but it isn't a bad option. It's both a cool concept and useful, so I'd still favor having it be available by default. Everyone feels this way about their favorite API, so take that for what it's worth. However, I do see people get concurrent asynchony wrong: failing to observe exceptions in a second task if awaiting the first fails or cancels, or using Task.Result because the WhenAll result type is unwieldy. There would be benefits to having a built-in and convenient syntax for doing this correctly. I wouldn't have to lead with, "So if you install this NuGet package... the correct syntax isn't terrible." |
If you do, please ensure it's not part of the shared framework. This isn't worth increasing the size of every self-contained app by ~27K (which is the size of that .dll), or even the shared framework by a similar amount. |
Won't tree shaking advance to the point where it can remove this kind of concern? |
I don't want to bet on it for things like this. If someone wants it, they can reference it, ship it with their app, etc. |
I wonder how much of that 27K will melt away when sharing headers, a string heap, AssemblyRef, TypeRef, and MemberRef tables, etc? 1.5K is strings, for instance, and the *Ref tables are busy. |
My point is, it's a ton of stuff. This is functionality that, IMHO, will benefit very few people (who may love it), nothing in core is going to depend on it, it's completely external to anything in the rest of the implementation and can easily be built on top, and every .NET customer shouldn't have to pay anything for it unless they want to use it. I do not understand why we're afraid of NuGet packages. Everything can't ship in the box, nor should it. We should be embracing the ecosystem, not hampering it by implying that things are only useful if they're shipped in the framework. |
I agree with not shipping everything inbox, yes. I'm not convinced that it will benefit very few people. If you use var (policy, preferences) = await (
GetPolicyAsync(policyId, cancellationToken),
GetPreferencesAsync(cancellationToken)
).ConfigureAwait(false); Today's option: // WhenAll requires all tasks to have the same result type,
// so Task.WhenAll(intTask, stringTask) will not compile.
var policyTask = GetPolicyAsync(policyId, cancellationToken);
var preferencesTask = GetPreferencesAsync(cancellationToken);
await Task.WhenAll((Task)policyTask, (Task)preferencesTask).ConfigureAwait(false);
var policy = policyTask.Result; // Exceptions are already thrown by the await, if any
var preferences = preferencesTask.Result; Or if the tasks happen to have the same result type: var tempName = await (
GetPolicyAsync(policyId, cancellationToken),
GetPreferencesAsync(cancellationToken)
).ConfigureAwait(false);
var policy = tempName[0];
var preferences = tempName[1]; This is what I think merits preference over Task.WhenAll. |
Suppose we stop at arity 3 instead of 10, and skip the void-resulting versions which hardly save you much? |
@stephentoub nuget can't be used in unity. I have frustrate so long that tuple cannot be used in unity. Until unity 2018 that contains C# 7. But nuget really cannot be used everywhere Everything that highly generic and could be used by many fields should be in the core as soon as possible If And so if you have custom dll that you would always copy to more than half of your project. And many people make their own version on similar feature. It should always be in the core. And might be having native syntax support on that things |
You can use nuget in Unity if you try hard. It is just not as straightforward as it should be. It is something you should be asking Unity to fix. It is not a good justification for adding APIs to the platform. |
@jkotas If we need to try hard to use it then defeat the purpose to use nuget as manager to make life easier. I could just copy dll directly or write my own code and that would be easier than trying hard to use nuget If nuget is that important it should be in the core already. So anywhere using dotnet will be able to use nuget. It might be the reason that it not natively support from the start that unity haven't integrate it into their system. |
@jkotas I think my side is the one who should say that, nuget is not a good justification for not adding APIs to the platform from the start. Core API should be just added when it is generic and high demand or people need to make a workaround to solve similar thing. Not just think that we have nuget so we could push anything in there. I just bring unity as the one example that nuget is not always available unlike core library |
nuget is a tool. It is not a runtime/framework API. I do not understand what you mean by having nuget in the core.
You are basically saying that the most frequently used nuget packages should be added to the platform. The are two problems with that: It would kill the nuget ecosystem; and it would make the platform really big over time. |
@jkotas In my opinion, nuget is misused for hosting a small utility library that should be in the core nuget is great and I use it on daily basis. But it actually should be used for third party service that required for some project, such as AWS and Google API or OpenCV or OpenGL. But not a core or module that more than 10% of project would like to use like JSON In other words. If every 10 projects will have a project using one same library. That library should be a core module. Just think that if there is 100000 projects using CI system. Every time each project deployed it will put a strain on nuget server to download the same library Unless nuget run on something like IPFS I will never think this approach is practical. Maybe we should have better modular system for the core library like this If you think json should not be core library, |
NuGet server is hosted in Azure. It can handle load like this just fine. Nothing to worry about. |
@Thaina NuGet is a fine solution for most things and this discussion seems to me to have gone off-topic. What I see is that this API should be preferred to Task.WhenAll any time there are non-void results, as shown above. Existing BCL alternatives are not pretty, but they will dominate if no other option ships on equal footing with Task.WhenAll. |
@jnm2 What I trying to say is nuget package is not a good solution for this feature. It should exist in BCL along with nuget is a good solution for many things but not these kind of core functionality |
To summarize the thread:
@jnm2 what you think? |
@tarekgh I just use unity as the obvious example that nuget does not really available everywhere in C# platform unlike core BCL. In reality people always prefer copying code for these small functionality Even @jnm2 himself admit that he does not use nuget for this
Do you think why? Because it really too small to rely on nuget. It is really the real situation in the world that nuget is not preferable in this functionality If you just conclude it like that then you miss the whole point |
@tarekgh Seems fair. To me it's really a decision whether consuming results from WhenAll the current way seems tasteful to you all, and that's entirely your prerogative. On the flip side, it is worth working uphill to change folks' reluctance to add a whole dependency to their project even if the alternative syntax is bad. It's another cross-cutting problem like Unity that needs to be worked independently. I do appreciate you asking me. I've made my case and I'm happy to abide by your decision. 👍 |
I think @jnm2 can answer this better but I think he may was not aware of the existence of this NuGet library. I don't believe nothing will prevent him of using the NuGet if decided to do so. @jnm2 thanks for your reply. I am going to close the issue and we can open it if anything new come up in the future. it was a good discussion, at least it documented some helper code and pointed to the NuGet package too. |
Just discovered this issue, only 2 1/2 years late, but I agree this API shouldn't be added not because it wouldn't be useful to a great many people but because it has little to no visibility. If this had been implemented, I would not have known until looking to suggest something similar. As such I think this should instead be added as a |
I've added my proposal above to dotnet/corefx#25756. |
@jnm2 has implemented
GetAwaiter
extension methods which enable convenient awaiting on tuples of tasks (up to some arity).For instance,
var (x, y) = await (thingX.OperationAsync(), thingY.OperationAsync());
@terrajobst @weshaggard @jkotas How do you recommend proceeding with such API proposals?
Do you need an API review before going into implementation?
If a PR is ok, I'm assuming that the Task-related extensions would go into https://github.com/dotnet/corefx/tree/master/src/Common/src/System/Threading/Tasks Is that appropriate?
Also, more generally, do you prefer to review such proposal piecemeal (one at a time) or all together (to get a well-thought API surface?). I remember some proposals for
Zip
andJoin
in LINQ APIs.Relates to dotnet/roslyn#16159
Proposal
There are three parts which can be considered separately.
1. Simple awaiting
Example:
Proposed API:
2. Configured awaiting
Example:
Proposed API:
3. Void-returning awaiting
Example:
Proposed API:
Higher arities
The above can be trivially generalized to any higher arity. Ten is tentatively suggested. I don’t recall seeing arities greater than four in practice. Eight would be weird because the return type would be
ValueTuple<T1, T2, T3, T4, T5, T6, T7, ValueTuple<T8>>
.If you actually need to parallelize five async operations, there’s nothing that performs better and is easier to write than the following:
Example:
Degenerate arity
The unary case could probably be left out for now since C# has no syntax for 1-tuples, but here it is for completeness:
Example:
Proposed API:
Questions
Should
TaskTupleExtensions.TupleTaskAwaiter
perhaps be renamedTaskTupleExtensions.TaskTupleAwaiter
?Entire API set
https://gist.github.com/jnm2/47a6a014831615bdb7b64a75f09a5799
The text was updated successfully, but these errors were encountered: