-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Provide a built-in way to create APM methods from a Task-based method #61729
Comments
Tagging subscribers to this area: @dotnet/area-system-threading-tasks Issue Details
The Framework does it that way in multiple places using the I suggest that the Framework should have a built-in way to do this. Possibly, the existing
|
Why not throw |
@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 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. |
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 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. |
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. |
Prototype: 9cc1d1d |
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);
} |
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:
Alternatively, we can introduce a new class:
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 onTask
and possiblyawait
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.The text was updated successfully, but these errors were encountered: