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

Reusing the same SocketAsyncEventArgs for different udp remote endpoints does not work anymore #97965

Closed
rmja opened this issue Feb 5, 2024 · 14 comments · Fixed by #98134
Closed
Assignees
Labels
area-System.Net.Sockets bug in-pr There is an active PR which will close this issue when it is merged regression-from-last-release
Milestone

Comments

@rmja
Copy link

rmja commented Feb 5, 2024

Description

It has been introduced in .NET 8 that the _socketAddress inside the SocketAsyncEventArgs is only serialized if it is null. This means that it is only serialized the first time the SocketAsyncEventArgs is used for a socket operation. However, e.g. in https://github.com/chronoxor/NetCoreServer/blob/master/source/NetCoreServer/UdpServer.cs#L692-L694 the SocketAsyncEventArgs is used multiple times for different remote endpoints.

This is what is going on in .NET 8:
https://github.com/dotnet/runtime/blob/v8.0.1/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L3098-L3107

This is the previous same file in .NET 7:
https://github.com/dotnet/runtime/blob/v7.0.15/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L3027

When comparing the two files, it seems that, if one changes the remote endpoint and calls e.g. Socket.SendToAsync() then it will not re-serialize the _socketAddress and the payload will not be transmitted to the socket address of the newly configured remote endpoint - instead they whill be transmitted to the first configured remote endpoint.

Reproduction Steps

  • Create a single instance of var ea = SocketAsyncEventArgs.
  • Call ea.RemoteEndPoint = firstEndpoint.
  • Call socket.SendToAsync(ea).
  • The data is correctly sent to firstEndpoint, now set a different remote endpoint ea.RemoteEndpoint = secondEndpoint
  • Call socket.SendToAsync(ea).
  • The data is not sent secondEndpoint , it is sent to the old firstEndpoint which is clearly not correct.

Expected behavior

It is expected that consequitive calls to e.g. Socket.SendToAsync() with the same instance of SocketAsyncEventArgs but with reconfigured RemoteEndpoint, actually transmits to different endpoints.

Actual behavior

All consequitive calls to e.g. Socket.SendToAsync() are transmitted to the remote endpoint configured when Socket.SendToAsync() is first called. This is verified in wireshark.

Regression?

This worked fine in .NET 7.

Known Workarounds

I made the following PR in NetCoreServer in case the re-use of SocketAsyncEventArgs for different remote endpoints is in fact not legal, see: chronoxor/NetCoreServer#284

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 5, 2024
@ghost
Copy link

ghost commented Feb 5, 2024

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

Issue Details

Description

It has been introduced in .NET 8 that the _socketAddress inside the SocketAsyncEventArgs is only serialized if it is null. This means that it is only serialized the first time the SocketAsyncEventArgs is used for a socket operation. However, e.g. in https://github.com/chronoxor/NetCoreServer/blob/master/source/NetCoreServer/UdpServer.cs#L692-L694 the SocketAsyncEventArgs is used multiple times for different remote endpoints.

This is what is going on in .NET 8:
https://github.com/dotnet/runtime/blob/v8.0.1/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L3098-L3107

This is the previous same file in .NET 7:
https://github.com/dotnet/runtime/blob/v7.0.15/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L3027

When comparing the two files, it seems that, if one changes the remote endpoint and calls e.g. Socket.SendToAsync() then it will not re-serialize the _socketAddress and the payload will not be transmitted to the socket address of the newly configured remote endpoint - instead they whill be transmitted to the first configured remote endpoint.

Reproduction Steps

  • Create a single instance of var ea = SocketAsyncEventArgs.
  • Call ea.RemoteEndPoint = firstEndpoint.
  • Call socket.SendToAsync(ea).
  • The data is correctly sent to firstEndpoint, now set a different remote endpoint ea.RemoteEndpoint = secondEndpoint
  • Call socket.SendToAsync(ea).
  • The data is not sent secondEndpoint , it is sent to the old firstEndpoint which is clearly not correct.

Expected behavior

It is expected that consequitive calls to e.g. Socket.SendToAsync() with the same instance of SocketAsyncEventArgs but with reconfigured RemoteEndpoint, actually transmits to different endpoints.

Actual behavior

All consequitive calls to e.g. Socket.SendToAsync() are transmitted to the remote endpoint configured when Socket.SendToAsync() is first called. This is verified in wireshark.

Regression?

This worked fine in .NET 7.

Known Workarounds

I made the following PR in NetCoreServer in case the re-use of SocketAsyncEventArgs for different remote endpoints is in fact not legal, see: chronoxor/NetCoreServer#284

Configuration

No response

Other information

No response

Author: rmja
Assignees: -
Labels:

area-System.Net.Sockets, untriaged

Milestone: -

@stephentoub
Copy link
Member

cc: @wfurt

@wfurt wfurt removed the untriaged New issue has not been triaged by the area owner label Feb 5, 2024
@wfurt wfurt added this to the 9.0.0 milestone Feb 5, 2024
@wfurt wfurt self-assigned this Feb 5, 2024
@wfurt
Copy link
Member

wfurt commented Feb 5, 2024

I'll take a look.

@rmja
Copy link
Author

rmja commented Feb 6, 2024

May I ask whether this was triaged to be a regression in .NET 8 or if the use of SocketAsyncEventArgs for different remote endpoints is a "too aggressive" optimization inside the NetCoreServer library? This would give some insight to whether chronoxor/NetCoreServer#284 should be merged or not, and maybe with a specific .NET 8 "fix" as I can see that this is not added to a .NET 8 patch milestone.

@wfurt
Copy link
Member

wfurt commented Feb 7, 2024

it is certainly unintended regression @rmja and the linked code looks fine to me. I should have PR up today to fix it. Once fixed and merged to main, we can reopen it for servicing request. I'm not sure what exact timing it would be.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 7, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 27, 2024
@wfurt
Copy link
Member

wfurt commented Mar 13, 2024

re-opening for backport

@gmeena
Copy link

gmeena commented May 14, 2024

it is certainly unintended regression @rmja and the linked code looks fine to me. I should have PR up today to fix it. Once fixed and merged to main, we can reopen it for servicing request. I'm not sure what exact timing it would be.

@wfurt do we know when it will be available in 8.0.x? any approximation

@wfurt
Copy link
Member

wfurt commented May 14, 2024

the fix missed 8.0.5 by day or so ;(
It should be in next update around mid June.

@gmeena
Copy link

gmeena commented May 14, 2024

the fix missed 8.0.5 by day or so ;( It should be in next update around mid June.

:( we just updated our services to .NET 8 and found this issue, now in spot. Any workaround?

@wfurt
Copy link
Member

wfurt commented May 14, 2024

use fresh SocketAsyncEventArgs for each round or have one used based on the RemoteEndPoint

@gmeena
Copy link

gmeena commented Jun 10, 2024

@wfurt Is this included in .NET 8.0.6 - May 28, 2024

@wfurt
Copy link
Member

wfurt commented Jun 10, 2024

no, my post had wrong version e.g. it missed 8.0.6 and it should be out mid June.

@wfurt
Copy link
Member

wfurt commented Jun 10, 2024

8.0 fixed by #99695. it should be out in 8.0.7

@karelz
Copy link
Member

karelz commented Jun 24, 2024

Fixed in main (9.0) in PR #98134 and in 8.0.7 (July release) in PR #99695.

@karelz karelz closed this as completed Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Sockets bug in-pr There is an active PR which will close this issue when it is merged regression-from-last-release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants