-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Consider adding TaskCompletionSource.SetFromTask(Task) #47998
Comments
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @tarekgh Issue DetailsBackground and MotivationA common operation when writing async code is to execute code that returns a task and complete. This usually involves creating a Proposed APInamespace System.Collections.Threading.Tasks
{
public class Task
{
+ public void ContinueWith(TaskCompletionSource tcs);
}
public class Task<T>
{
+ public void ContinueWith(TaskCompletionSource<T> tcs);
}
} Usage Examplespublic class Connection
{
private TaskCompletionSource _tcs = new();
private CancealltionTokenSource _cts = new();
public Task Run()
{
ThreadPool.QueueUserWorkItem(_ => Start());
return _tcs.Task;
}
public void Start()
{
NetworkHelper.StartConnectionAsync(_cts).ContinueWith(tcs);
}
// ...
} This is a common pattern when you need to kick off an async operation and can't directly wait the operation inline. Alternative DesignsDo this manually. public static class TaskExtensions
{
public void ContinueWith(this Task task, TaskCompletionSource tcs)
{
async Task Continue()
{
try
{
await task;
tcs.TrySetResult();
}
catch (OperationCancelledException ex)
{
tcs.TrySetCancelled(ex.Cancelled);
}
catch (Exception ex)
{
tcs.TrySetException(ex);
}
}
_ = Continue();
}
} RisksNone.
|
We've considered such a thing in the past, e.g. tcs.SetFromTask(task). But we frequently found that there was a better way of writing it. For example, in your usage example: public class Connection
{
private TaskCompletionSource _tcs = new();
private CancealltionTokenSource _cts = new();
public Task Run()
{
ThreadPool.QueueUserWorkItem(_ => Start());
return _tcs.Task;
}
public void Start()
{
NetworkHelper.StartConnectionAsync(_cts).ContinueWith(tcs);
}
// ...
} passing the tcs into StartConnectionAsync and having it complete it. public class Connection
{
private TaskCompletionSource _tcs = new();
private CancealltionTokenSource _cts = new();
public Task Run()
{
ThreadPool.QueueUserWorkItem(_ => Start());
return _tcs.Task;
}
public void Start()
{
NetworkHelper.StartConnectionAsync(_tcs, _cts);
}
// ...
} or if StartConnectionAsync actually returned a Task (since I know how much you love async void methods), just using Task.Run: public class Connection
{
private CancealltionTokenSource _cts = new();
public Task Run() => Task.Run(() => Start());
public Task Start() => NetworkHelper.StartConnectionAsync(_cts);
// ...
} which does such transfer automatically. |
|
You can of course use SuppressFlow. Yes, it's more code, but such a SetFromTask would need to be used enough to make it worthwhile. Are you using UnsafeQueueUserWorkItem purely for the ExecutionContext behavior? Or do you actually need to queue the invocation of the async operation for some reason? This still seems to me to be too niche to warrant exposing. The sole place we have such a SetFromTask method is in the implementation of Task.Run itself. How many places would you actually use this? |
I need both behaviors.
Yea, it's not a big deal. I wrote a helper but I remembered that I also wrote a helper when we did the original SignalR (https:/SignalR/SignalR/blob/75126981cd165fb57db4e38e9379e651fec1ecdf/src/Microsoft.AspNet.SignalR.Core/TaskAsyncHelper.cs#L315)
I'll look around. I don't think it's critical at the moment, I filed this because it came up again in code I was writing and I didn't the first time |
@davidfowl an existing alternative of the proposed API is to use a cold nested task and the public class Connection
{
private Task<Task> _taskTask;
private CancellationTokenSource _cts = new();
public Connection()
{
_taskTask = new Task<Task>(() => NetworkHelper.StartConnectionAsync(_cts));
}
public Task Run()
{
ThreadPool.QueueUserWorkItem(_ => _taskTask.RunSynchronously(TaskScheduler.Default));
return _taskTask.Unwrap();
}
// ...
} |
Man I forgot about Unwrap m. Async/await made me forget past trauma. |
Do cold tasks capture the execution context? |
I wish I knew that! |
Yes. ExecutionContext is captured in Task's delegate-based ctor. |
@davidfowl based on the answers, do you still believe we should continue pursuing this idea, or can we go ahead and close the issue? |
Future will do for now |
I was working recently on 3 different projects at Microsoft - BuildXL, a distributed cache and AnyBuild. And I needed Of course, this is an example of selection bias, but I really think it would be helpful to have the API @davidfowl proposed. |
I added these when I forgot that this proposal was building in the continuation into the operation. They make zero sense in that case. I also think that the SetFromTask name doesn't convey what it's actually being done here... my intuition is that it's just transferring the completion state of an existing task, just as eg SetResult is storing some existing result, in which case Try would make sense. And I don't think we can add the Try that means one thing and the non-Try that means something else. It's also not clear to me what the behavior of these methods would be if someone calls SetFromTask, then separately completes the TCS, and then the task passed to SetFromTask completes. |
This is a good question. I think the normal behavior applies. The first one to call Try* wins. Maybe that means we need to change the name and also handle what happens if you call SetFromTask multiple times on different tasks ? |
I think it'd be much cleaner if {Try}SetFromTask was simply about transferring the state of an already-completed task over to a TCS. If you wanted that to happen when the task completes (and you weren't otherwise already registering a continuation with it for other reasons), you'd hook up a continuation explicitly, e.g. rather than: tcs.SetFromTask(task); that would implicitly add a continuation to task.ContinueWith(tcs.SetFromTask); or: await task.NoThrow();
tcs.SetFromTask(task); Yes, they're arguably a bit more complicated to use, but I also think they're more easily explainable, they address the issue of what to do if the TCS is completed before a previously SetFromTask'd task completes, and it leaves the semantics of SetFromTask being effectively the same as the existing SetResult/Exception/Canceled. You get to be very explicit about what to do if you lose a race condition to complete the TCS, you get to manually control the continuation and can then do things like cancel it or do additional work as part of the continuation, etc. |
namespace System.Collections.Threading.Tasks
{
public partial class TaskCompletionSource
{
public void SetFromTask(Task completedTask);
public bool TrySetFromTask(Task completedTask);
}
public partial class TaskCompletionSource<T>
{
public void SetFromTask(Task<T> completedTask);
public bool TrySetFromTask(Task<T> completedTask);
}
} |
EDITED by @stephentoub on 1/21/2023:
These methods require that the task argument has already completed and transfer the result/exception/cancellation state over to the TCS's Task.
Background and Motivation
A common operation when writing async code is to execute code that returns a task and complete. This usually involves creating a
TaskCompletionSource
to represent the operation, then executing the operation that returns a task at point in the future, then setting the result on completion to either failed, cancelled or success.Proposed API
Usage Examples
This is a common pattern when you need to kick off an async operation and can't directly wait the operation inline.
Alternative Designs
Do this manually.
Risks
None.
The text was updated successfully, but these errors were encountered: