Skip to content
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

Sockets cleanup #1351

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Sockets cleanup #1351

wants to merge 5 commits into from

Conversation

Rob-Hague
Copy link
Collaborator

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

  • Use "using" and ManualResetEventSlim in Connect
  • Delete unused and unnecessary methods

4. Add a loop to SocketAbstraction.ReadAsync

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

@Rob-Hague Rob-Hague marked this pull request as draft March 9, 2024 10:04
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. 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.
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.
* Use "using" and ManualResetEventSlim in Connect
* Delete unused and unnecessary methods
In order to ensure the buffer is read completely, as in SocketAbstraction.Read
@WojciechNagorski
Copy link
Collaborator

Do you want to finish something here?

@Rob-Hague Rob-Hague marked this pull request as ready for review March 14, 2024 12:55
@Rob-Hague
Copy link
Collaborator Author

I think it's ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants