Runtime: Socket.ReceiveAsync and other methods don't support sync completion

Created on 17 Nov 2016 · 15Comments · Source: dotnet/runtime

On Windows, it is possible to use the following WIN API function to enable WSARecv and other calls on the socket to complete synchronously when possible and avoid triggering IOCP completion: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365538(v=vs.85).aspx
It is possible to do even now, e.g.:
``` C#
public static bool SetFileCompletionNotificationModes(this Socket socket, FileCompletionNotificationModes flags)
{
SetFileCompletionNotificationModes(socket.Handle, flags);
}

[DllImport("kernel32.dll")]
internal static extern bool SetFileCompletionNotificationModes([In] IntPtr fileHandle, [In] FileCompletionNotificationModes flags);

[Flags]
public enum FileCompletionNotificationModes
{
SkipCompletionPortOnSuccess = 1,
SkipSetEventOnHandle = 2
}

The problem is in ReceiveAsync implementation on this line: https://github.com/dotnet/corefx/blob/5e89fcf41481481e7b673b10ea7644ce56776bd9/src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L4356:
``` C#
if (socketError != SocketError.Success && socketError != SocketError.IOPending)
{
    e.FinishOperationSyncFailure(socketError, bytesTransferred, flags);
    retval = false;
}
else
{
    retval = true;
}

Basically, if returned result is not Success or IOPending then it is a sync completion with error. If it is Success or IOPending - it will return true.
WSARecv will not return SUCCESS for IOCP-based call unless SetFileCompletionNotificationModes was called for the socket setting the flags above. So in default operation mode check for inequality to SocketError.Success is harmless (but redundant) and is always true. Once you set notification modes though, WSARecv may return SUCCESS in case of sync completion and then it won't invoke callback on SAEA and per code above will return true.
Proposal is to change behavior of ReceiveAsync and other IOCP-based methods to support synchronous completion. The code above would change as follows:
C# if (socketError == SocketError.Success) { e.FinishOperationSuccess(SocketError.Success, bytesTransferred, flags, completedSynchronously: true); retval = false; } else { socketError = (SocketError)Marshal.GetLastWin32Error(); if (socketError == SocketError.IOPending) { retval = true; } else { e.FinishOperationSyncFailure(socketError, bytesTransferred, SocketFlags.None); retval = false; } }
Another change not shown here is to SocketAsyncEventArgs.FinishOperationSuccess, adding parameter bool completedSynchronously that if true would skip raising SAEA.Completed event.

This change and support for SetFileCompletionNotificationModes yield significant performance improvement for scenarios with high throughput over a small number of sockets.

Let me know if the change is something you can consider to take and I'd post a PR.

area-System.Net.Sockets enhancement

Source

nayato

Most helpful comment

also I don't think it will be OK to enable the flag on socket by default as I guess there is a lot of code out there relying on current behavior. How would you want to expose it?

@cipop / @davidsh should comment, but I actually think we should fix it in the current implementation. I'm interested in other opinions, too, though... @jkotas? @geoffkizer? @pgavlin? My view is that it's simply a bug in the implementation today that operations complete asynchronously when they could complete synchronously. The docs for SocketAsyncEventArgs are already very clear that operations can complete synchronously, with the API explicitly designed for that, so it's effectively a bug in someone's code if they're not already planning for that possibility.