Provide a built-in way to create APM methods from a Task-based method

[GSPP]

EDITED by @stephentoub on 1/26/2023:

We don't want to reinvigorate the legacy pattern, so we can bury these helpers now as statics on the IAsyncResult interface itself... the only reason to reach for that interface is if you're using that legacy pattern, and these methods naturally fit there:

namespace System
{
    public interface IAsyncResult
    {
+        public static IAsyncResult BeginFromTask(Task task, AsyncCallback? callback, object? state);
+        public static void EndFromTask(IAsyncResult asyncResult);
+        public static TResult EndFromTask<TResult>(IAsyncResult asyncResult);
+        public static Task UnwrapTask(IAsyncResult asyncResult);
+        public static Task<TResult> UnwrapTask<TResult>(IAsyncResult asyncResult);
    }
}

Alternatively, we can introduce a new class:

namespace System.Threading.Tasks
{
+    public static class TaskToAsyncResult
+    {
+        public static IAsyncResult Begin(Task task, AsyncCallback? callback, object? state);
+        public static void End(IAsyncResult asyncResult);
+        public static TResult End<TResult>(IAsyncResult asyncResult);
+        public static Task Unwrap(IAsyncResult asyncResult);
+        public static Task<TResult> Unwrap<TResult>(IAsyncResult asyncResult);
+    }
}

---

Stream provides both APM and Task methods for reading and writing. When creating a custom stream, you ideally override them both and provide a functional implementation. It is desirable to be able to implement this based on Task and possibly await just once and reuse that code for the APM methods.

The Framework does it that way in multiple places using the TaskToApm class (https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/Threading/Tasks/TaskToApm.cs).

I suggest that the Framework should have a built-in way to do this. Possibly, the existing TaskToApm class can just be polished and made public.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-threading-tasks See info in area-owners.md if you want to be subscribed.

Issue Details

---

`Stream` provides both APM and Task methods for reading and writing. When creating a custom stream, you ideally override them both and provide a functional implementation. It is desirable to be able to implement this based on `Task` and possibly `await` just once and reuse that code for the APM methods. The Framework does it that way in multiple places using the `TaskToApm` class (https://github.com/dotnet/runtime/blob/main/src/libraries/Common/src/System/Threading/Tasks/TaskToApm.cs). I suggest that the Framework should have a built-in way to do this. Possibly, the existing `TaskToApm` class can just be polished and made public.

Author:	GSPP
Assignees:	-
Labels:	`area-System.Threading.Tasks`, `untriaged`
Milestone:	-

[teo-tsirpanis]

Why not throw NotSupportedException on user code classes overriding APM methods, especially when they only target modern .NET? How many widely used libraries are actually using APM?

[sakno]

@teo-tsirpanis , because legacy code that just works flow through various version of enterprise application. Application itself can be upgraded to later versions of .NET but still have some portions of that code. The most frequent situation is Stream. Moving from BeginXXX/EndXXX to ReadAsync/WriteAsync is not easy. For instance, the application may have two logical parts: consumer and producer of Stream. Producer can be upgraded to modern API while consumer not. In this case, the consumer still use APM methods.

In dotNext library I have to provide implementation of APM methods for various streams because it's requested by users. Also, we can do a simple query over GitHub and find related issues in SSH.NET (~2.7K watchers), websocket-sharp (~4.4K watchers) and others.

[GSPP]

When a Stream fails to react correctly to a part of its contract, it fails to meet the Liskov substitution principle. In concrete cases, you can often get away with it. The cost, though, is a long-term technical debt that makes the application brittle in the face of modification. The "clean" way to go is to implement the full Stream contract and provide a high-quality, production-viable implementation of everything. Anything else is a hack that can work concretely at the cost of architectural soundness.

Concretely, even the BCL uses APM methods internally. 3rd-party libraries might as well. Company-internal code might very well do that. Abstractly, failing to provide a viable implementation for everything poses a latent risk and might cause future costs.

[stephentoub]

I've marked it as ready for review in order to have a discussion about it in API review. I'm quite hesitant to expose new APIs to make it easier to implement a very legacy pattern we don't want anyone consuming moving forward. I understand the argument that existing code still consumes it.

[stephentoub]

Prototype: https://github.com/dotnet/runtime/commit/9cc1d1d532db63dad9b15c9e701f58fbb75803b7

[terrajobst]

Video

We concluded that having a separate type will be easier should we need to ship this downlevel (and there is some evidence that suggests we might want to do that).

namespace System.Threading.Tasks;

public static class TaskToAsyncResult
{
    public static IAsyncResult Begin(Task task, AsyncCallback? callback, object? state);
    public static void End(IAsyncResult asyncResult);
    public static TResult End<TResult>(IAsyncResult asyncResult);
    public static Task Unwrap(IAsyncResult asyncResult);
    public static Task<TResult> Unwrap<TResult>(IAsyncResult asyncResult);
}