-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Socket.Windows: support ConnectAsync(SocketAsyncEventArgs) for UDP, and Unix sockets #33674
Changes from all commits
08ce33a
1796b80
dce3be8
abf67a1
0fc45ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,14 @@ internal unsafe SocketError DoOperationAccept(Socket socket, SafeSocketHandle ha | |
} | ||
|
||
internal unsafe SocketError DoOperationConnect(Socket socket, SafeSocketHandle handle) | ||
{ | ||
// Called for connectionless protocols. | ||
SocketError socketError = SocketPal.Connect(handle, _socketAddress!.Buffer, _socketAddress.Size); | ||
FinishOperationSync(socketError, 0, SocketFlags.None); | ||
return socketError; | ||
} | ||
|
||
internal unsafe SocketError DoOperationConnectEx(Socket socket, SafeSocketHandle handle) | ||
{ | ||
// ConnectEx uses a sockaddr buffer containing the remote address to which to connect. | ||
// It can also optionally take a single buffer of data to send after the connection is complete. | ||
|
@@ -1160,6 +1168,13 @@ private unsafe SocketError FinishOperationConnect() | |
{ | ||
try | ||
{ | ||
if (_currentSocket!.SocketType != SocketType.Stream) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @antonfirsov @scalablecory also give this condition some thought. Personally, I'm fine if CI passes. You may have a better understanding of what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should match the logic for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking the same thing, though it seems the stream unix socket, which doesn't support ConnectEx, doesn't give an error when we set this socket option. |
||
{ | ||
// With connectionless sockets, regular connect is used instead of ConnectEx, | ||
// attempting to set SO_UPDATE_CONNECT_CONTEXT will result in an error. | ||
return SocketError.Success; | ||
} | ||
|
||
// Update the socket context. | ||
SocketError socketError = Interop.Winsock.setsockopt( | ||
_currentSocket!.SafeHandle, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if @stephentoub's #33674 (comment) also applies to
AddressFamily
? Wouldn't it be more future proof to use the condition(remoteEP.AddressFamily == AddressFamily.InterNetwork || remoteEP.AddressFamily == AddressFamily.InterNetworkV6)
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no preference for
remoteEP.GetType() == typeof(IPEndPoint)
vs(remoteEP.AddressFamily == AddressFamily.InterNetwork || remoteEP.AddressFamily == AddressFamily.InterNetworkV6)
, so I kept the check as it was.