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.

Sign up for GitHub

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

Jump to bottom

add SendTo/ReceiveFrom with SocketAddress #88970

Merged
merged 18 commits into from
Aug 1, 2023
Merged

add SendTo/ReceiveFrom with SocketAddress #88970

merged 18 commits into from
Aug 1, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 16, 2023 •
edited
Loading

And uses new SocketAddress API. That spanifies some of the PAL code as prep for removal of Internal.SocketAddress.

fixes #86872
contributes to #87397
contributes to #30797

@ghost ghost assigned wfurt Jul 16, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jul 16, 2023

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

Issue Details

And uses new SocketAddress API. That spanifies some of the PAL code as prep for removal of Internal.SocketAddress.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Sockets, new-api-needs-documentation
Milestone: -

This was referenced Jul 17, 2023
Closed
Closed
This was referenced Jul 17, 2023
Closed
Closed
Closed
@build-analysis build-analysis bot mentioned this pull request Jul 18, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Jul 19, 2023
Open
@wfurt wfurt changed the title Poc: add SendTo/ReceiveFrom with SocketAddress add SendTo/ReceiveFrom with SocketAddress Jul 19, 2023
@wfurt wfurt requested a review from a team July 19, 2023 16:52
@build-analysis build-analysis bot mentioned this pull request Jul 20, 2023
Open
5 tasks
stephentoub
stephentoub reviewed Jul 27, 2023
View reviewed changes
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs
@@ -763,7 +763,7 @@ internal unsafe SocketError DoOperationSendToSingleBuffer(SafeSocketHandle handl
1,
out int bytesTransferred,
_socketFlags,
_socketAddress!.Buffer.AsSpan(),
_socketAddress!.InternalBuffer.AsSpan(),
Copy link
Member

Choose a reason for hiding this comment

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

Same question as earlier for these... we don't need to splice it down to Size?

stephentoub
stephentoub reviewed Jul 27, 2023
View reviewed changes
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs
// We can fail to get peer address on TCP
socketAddressLen = socketAddress.Length;
SocketAddressPal.Clear(socketAddress);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this a bug fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure but possibly yes. On Windows we seems to get the address but it fails on Linux. Perhaps we can also check if this is TCP and get the value from RemoteEndPoint? I can write issue for improvement and investigation.
I had trouble because setting Size zero would throw. Alternatively we can allow it to indicate that the buffer does not have any valid address.

stephentoub
stephentoub reviewed Jul 27, 2023
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Other than my comments/questions, LGTM.

@stephentoub
Copy link
Member

I assume SendToAsync / ReceiveFromAsync are coming in a follow-up PR?

@wfurt
Copy link
Member Author

wfurt commented Jul 27, 2023

I assume SendToAsync / ReceiveFromAsync are coming in a follow-up PR?

yes. That needs more work to clean up Internal.SocketAddress in socket code.

@wfurt
Copy link
Member Author

wfurt commented Jul 28, 2023

this should be ready for another review pass @stephentoub

stephentoub
stephentoub reviewed Aug 1, 2023
View reviewed changes
src/libraries/Common/src/Interop/Windows/WinSock/Interop.WSAConnect.cs Outdated
@@ -12,7 +12,7 @@ internal static partial class Winsock
[LibraryImport(Interop.Libraries.Ws2_32, SetLastError = true)]
internal static partial SocketError WSAConnect(
SafeSocketHandle socketHandle,
byte[] socketAddress,
Span<byte> socketAddress,
Copy link
Member

@stephentoub stephentoub Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we make the actual LibraryImport private, with an internal wrapper that doesn't take socketAddressSize? e.g.

[LibraryImport(Interop.Libraries.Ws2_32, SetLastError = true)]
private static partial SocketError WSAConnect(
    SafeSocketHandle socketHandle,
    Span<byte> socketAddress,
    int socketAddressSize,
    IntPtr inBuffer,
    IntPtr outBuffer,
    IntPtr sQOS,
    IntPtr gQOS);

internal static SocketError WSAConnect(
    SafeSocketHandle socketHandle,
    Span<byte> socketAddress,
    IntPtr inBuffer,
    IntPtr outBuffer,
    IntPtr sQOS,
    IntPtr gQOS) =>
    WSAConnect(socketHandle, socketAddress, socketAddress.Length, inBuffer, outBuffer, sQOS, gQOS);

?

Though reading further that might make it a bit inconsistent with some of the other overloads. It's just a bit strange to see both the span and length passed in.

@wfurt
Copy link
Member Author

wfurt commented Aug 1, 2023

perf check

Method Job Toolchain Mean Error StdDev Median Min Max Ratio RatioSD Allocated Alloc Ratio
ConnectAcceptAsync Job-QAPYHK \PR \corerun.exe 416.5 us 4.92 us 4.61 us 415.9 us 408.6 us 426.5 us 1.01 0.01 1370 B 0.99
ConnectAcceptAsync Job-ZQMRZG \main\corerun.exe 411.2 us 3.12 us 2.77 us 410.7 us 407.7 us 416.6 us 1.00 0.00 1386 B 1.00
SendAsyncThenReceiveAsync_Task Job-QAPYHK \PR \corerun.exe 271,064.3 us 4,562.00 us 4,267.30 us 269,604.6 us 265,718.7 us 277,834.9 us 1.01 0.02 752 B 1.00
SendAsyncThenReceiveAsync_Task Job-ZQMRZG \main\corerun.exe 267,751.9 us 1,734.53 us 1,622.48 us 267,574.0 us 265,252.0 us 270,458.9 us 1.00 0.00 752 B 1.00
ReceiveAsyncThenSendAsync_Task Job-QAPYHK \PR \corerun.exe 351,264.2 us 1,238.80 us 1,158.77 us 351,484.9 us 349,380.1 us 353,069.9 us 1.02 0.01 1280 B 1.29
ReceiveAsyncThenSendAsync_Task Job-ZQMRZG \main\corerun.exe 345,229.4 us 1,554.32 us 1,453.92 us 345,121.6 us 343,334.1 us 348,427.2 us 1.00 0.00 992 B 1.00
SendAsyncThenReceiveAsync_SocketAsyncEventArgs Job-QAPYHK \PR \corerun.exe 262,947.6 us 2,276.07 us 2,129.04 us 261,991.3 us 260,505.5 us 266,591.9 us 0.96 0.01 1920 B 1.00
SendAsyncThenReceiveAsync_SocketAsyncEventArgs Job-ZQMRZG \main\corerun.exe 274,286.3 us 2,102.03 us 1,966.24 us 274,853.0 us 270,239.3 us 277,223.5 us 1.00 0.00 1920 B 1.00
ReceiveAsyncThenSendAsync_SocketAsyncEventArgs Job-QAPYHK \PR \corerun.exe 331,678.8 us 990.62 us 926.63 us 331,653.9 us 329,933.3 us 333,237.0 us 1.00 0.01 3088 B 1.33
ReceiveAsyncThenSendAsync_SocketAsyncEventArgs Job-ZQMRZG \main\corerun.exe 332,023.6 us 1,PR 1 us 1,428.75 us 332,203.7 us 329,059.4 us 334,365.6 us 1.00 0.00 2320 B 1.00

@wfurt wfurt merged commit e2c6fcf into dotnet:main Aug 1, 2023
@karelz karelz added this to the 8.0.0 milestone Aug 1, 2023
This was referenced Aug 2, 2023
Merged
Merged
Merged
@MihaZupan MihaZupan mentioned this pull request Aug 7, 2023
Closed
@wfurt wfurt mentioned this pull request Aug 10, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

Assignees

@wfurt wfurt

Labels
area-System.Net.Sockets new-api-needs-documentation
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

[API Proposal]: make SocketAddress more useful
3 participants
@wfurt @stephentoub @karelz