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

fix SendTo with SocketAsyncEventArgs #98134

Merged
merged 6 commits into from
Feb 27, 2024
Merged

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Feb 7, 2024

Fixes #97965 and #99863 -- a regression from PR #90086 (in 8.0).

With the new SocketAddress changes the task based IO worked because SendToAsync explicitly sets saea._socketAddress to null. But there is no access to it for anybody using SocketAsyncEventArgs directly.

In the past, RemoteEndPoint was only user accessible variable and _socketAddress was always managed internally. With the new API SendTo and ReceiveFrom can use SocketAddress directly and reference would be also set on SocketAsyncEventArgs. As I realized, 1) we should make no assumption about and and we should not try to use it for anything else than just the particular call and 2) SocketAsyncEventArgs can be used directly so any related logic really needs to be at the bottom and not in the layers above.

With that I made changes to set RemoteEndPoint to null to indicate that SendTo and ReceiveFrom was invoked with user provided SocketAddress. We will also detach it from SocketAsyncEventArgs in that case so it can be re-used for another operations safely.
On other cases we can keep it around and simply override _socketAddress as we see fit instead of allocating new one every time.

@wfurt wfurt added this to the 9.0.0 milestone Feb 7, 2024
@wfurt wfurt requested a review from a team February 7, 2024 22:47
@wfurt wfurt self-assigned this Feb 7, 2024
@ghost
Copy link

ghost commented Feb 7, 2024

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

Issue Details

this fixes regression from #90086 and fixes #97965.

With the new SocketAddress changes the task based IO worked because SendToAsync explicitly sets saea._socketAddress to null. But there is no access to it for anybody using SocketAsyncEventArgs directly.

In the past, RemoteEndPoint was only user accessible variable and _socketAddress was always managed internally. With the new API SendTo and ReceiveFrom can use SocketAddress directly and reference would be also set on SocketAsyncEventArgs. As I realized, 1) we should make no assumption about and and we should not try to use it for anything else than just the particular call and 2) SocketAsyncEventArgs can be used directly so any related logic really needs to be at the bottom and not in the layers above.

With that I made changes to set RemoteEndPoint to null to indicate that SendTo and ReceiveFrom was invoked with user provided SocketAddress. We will also detach it from SocketAsyncEventArgs in that case so it can be re-used for another operations safely.
On other cases we can keep it around and simply override _socketAddress as we see fit instead of allocating new one every time.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Sockets

Milestone: 9.0.0

finally
{
// detach user provided SA so we do not accidentally stomp on it later.
saea._socketAddress = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to null it here because it's only used synchronously as part of the send operation and not asynchronously?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. We will make _socketAddress before first try. f we do not finish synchronously on Unix, it would be copied to queued *SendOperation. For Windows, the const sockaddr *lpTo is IN for WSASendTo and AFAIK it does not need to be preserved until the overlapped IO is completed.

if (_remoteEndPoint == null)
{
// detach user provided SA as it was updated in place.
_socketAddress = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only two operations it would be set for are ReceiveFrom and SendTo, and we already handled the SendTo case on the synchronous start path?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. For SendTo we clear it when operation starts, for ReceiveFrom we do it when the operation is finished.

@wfurt wfurt merged commit f9637f1 into dotnet:main Feb 27, 2024
100 of 107 checks passed
@wfurt wfurt deleted the SocketAsyncEventArgs branch February 27, 2024 16:50
@wfurt
Copy link
Member Author

wfurt commented Mar 13, 2024

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8268148999

@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reusing the same SocketAsyncEventArgs for different udp remote endpoints does not work anymore
2 participants