sshnet / SSH.NET

SSH.NET is a Secure Shell (SSH) library for .NET, optimized for parallelism.
http://sshnet.github.io/SSH.NET/
MIT License
3.88k stars 917 forks source link

Sockets cleanup #1351

Open Rob-Hague opened 3 months ago

Rob-Hague commented 3 months ago

1. Replace AwaitableSocketAsyncEventArgs in SocketExtensions

The existing AwaitableSocketAsyncEventArgs is useful in principal for being reusable in order to save on allocations. However, we don't reuse it and the implementation is flawed (#913). Instead, use implementations based on TaskCompletionSource, and add a SendAsync method.

Because sockets are only natively cancellable on modern .NET, I was torn between 3 options for cancellation on the targets which use SocketExtensions:

  1. Do not respect the CancellationToken once the socket operation has started. I believe this is what earlier versions of .NET Core did when CancellationToken overloads were first added via SocketTaskExtensions.
  2. Do not close the socket upon cancellation, meaning the socket operation continues to run after the Task has completed. This is what the previous implementation effectively does.
  3. Close the socket when the CancellationToken is cancelled, in order to stop the socket operation. The behaviour of a socket after proper cancellation is undefined(?), so in any case it should not make sense to use the socket after triggering cancellation.

I felt that option 2 was the worst of them. This iteration goes for option 3.

2. Remove "IsErrorResumable" and SocketAbstraction.Send{Async}

Some methods in SocketAbstraction have code to retry a socket operation if it returns certain error codes. However AFAIK, these errors are only pertinent to nonblocking sockets, which we do not use.

For blocking sockets, Socket.Send{Async} only returns when all of the bytes are sent. There is no need for a loop.

These changes combined mean there is no need for Send methods in SocketAbstraction.

3. Cleanup SocketAbstraction

4. Add a loop to SocketAbstraction.ReadAsync

In order to ensure the buffer is read completely, as in SocketAbstraction.Read

WojciechNagorski commented 3 months ago

Do you want to finish something here?

Rob-Hague commented 3 months ago

I think it's ready for review