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

AcceptAsync with existing Socket not implemented on Unix #1483

Closed
stephentoub opened this issue Mar 16, 2017 · 8 comments · Fixed by #73499
Closed

AcceptAsync with existing Socket not implemented on Unix #1483

stephentoub opened this issue Mar 16, 2017 · 8 comments · Fixed by #73499
Assignees
Labels
area-System.Net.Sockets enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Milestone

Comments

@stephentoub
Copy link
Member

Windows has built-in support for accepting into an existing socket, something not available on Unix. But we can approximate it in the .NET layer on Unix, e.g. by accepting into a new socket and transferring over the SafeCloseHandle and related state. This will allow the same APIs to be used cross-platform so that code running on Windows gets the intended benefits while not having to fork based on the OS.

@karelz karelz transferred this issue from dotnet/corefx Jan 9, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Sockets untriaged New issue has not been triaged by the area owner labels Jan 9, 2020
@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Jan 9, 2020
@karelz karelz added this to the Future milestone Jan 9, 2020
@marcusturewicz
Copy link
Contributor

So is the idea to implement the equivalent of:

internal unsafe bool AcceptEx(SafeSocketHandle listenSocketHandle,
SafeSocketHandle acceptSocketHandle,
IntPtr buffer,
int len,
int localAddressLength,
int remoteAddressLength,
out int bytesReceived,
NativeOverlapped* overlapped)
{
EnsureDynamicWinsockMethods();
AcceptExDelegate acceptEx = _dynamicWinsockMethods!.GetDelegate<AcceptExDelegate>(listenSocketHandle);
return acceptEx(listenSocketHandle,
acceptSocketHandle,
buffer,
len,
localAddressLength,
remoteAddressLength,
out bytesReceived,
overlapped);
}

in Socket.Unix.cs ?

@stephentoub
Copy link
Member Author

Today on Unix we just throw a PlatformNotSupportedException if a non-null accept Socket is supplied:

private Socket? GetOrCreateAcceptSocket(Socket? acceptSocket, bool unused, string propertyName, out SafeSocketHandle? handle)
{
// AcceptSocket is not supported on Unix.
if (acceptSocket != null)
{
throw new PlatformNotSupportedException(SR.PlatformNotSupported_AcceptSocket);
}
handle = null;
return null;
}

Instead, we would need to:

  • Allow a non-null Socket, but put it aside for the moment
  • Perform the accept using a new Socket, as the Unix implementation already does today
  • Copy the relevant state from the new Socket into the supplied non-null accept Socket, e.g. most of the fields on the Socket instance
  • Clear out / suppress finalization the temporary accept socket that was created

Or something along those lines.

@marcusturewicz
Copy link
Contributor

Ok. I'll have a go at implementing this, including tests.

@stephentoub
Copy link
Member Author

Thanks!

@marcusturewicz
Copy link
Contributor

Is this on the right track?

        private Socket? GetOrCreateAcceptSocket(Socket? acceptSocket, bool unused, string propertyName, out SafeSocketHandle? handle)
        {
            if (acceptSocket != null)
            {
                // Accept into new Socket
                using var socket = new Socket(_addressFamily, _socketType, _protocolType);

                // Copy fields to acceptSocket
                acceptSocket._addressFamily = socket._addressFamily;
                acceptSocket._cachedTaskEventArgs = socket._cachedTaskEventArgs;
                acceptSocket._caches = socket._caches;
                acceptSocket._closeTimeout = socket._closeTimeout;
                acceptSocket._disposed = socket._disposed;
                acceptSocket._handle = socket._handle;
                acceptSocket._isConnected = socket._isConnected;
                acceptSocket._isDisconnected = socket._isDisconnected;
                acceptSocket._isListening = socket._isListening;
                acceptSocket._lastReceiveHandle = socket._lastReceiveHandle;
                acceptSocket._lastReceiveThread = socket._lastReceiveThread;
                acceptSocket._lastReceiveTick = socket._lastReceiveTick;
                acceptSocket._nonBlockingConnectInProgress = socket._nonBlockingConnectInProgress;
                acceptSocket._nonBlockingConnectRightEndPoint = socket._nonBlockingConnectRightEndPoint;
                acceptSocket._protocolType = socket._protocolType;
                acceptSocket._receivingPacketInformation = socket._receivingPacketInformation;
                acceptSocket._remoteEndPoint = socket._remoteEndPoint;
                acceptSocket._rightEndPoint = socket._rightEndPoint;
                acceptSocket._socketType = socket._socketType;
                acceptSocket._willBlock = socket._willBlock;
                acceptSocket._willBlockInternal = socket._willBlockInternal;

                handle = acceptSocket._handle;

                return acceptSocket;
            }

            handle = null;
            return null;
        }

@stephentoub
Copy link
Member Author

stephentoub commented Apr 13, 2020

Yes and no. Yes, that's the kind of copying to be done. No, this isn't the right place to do it: it would need to be after the accept had already happened. You also need to be careful not to tear down the new socket when cleaning up the temporary one.

@liveans
Copy link
Member

liveans commented Aug 4, 2022

I'm going to take a look at this issue @karelz

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 5, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 26, 2022
@davidfowl
Copy link
Member

We should fix the milestone right?

@wfurt wfurt modified the milestones: Future, 8.0.0 Aug 26, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets enhancement Product code improvement that does NOT require public API changes/additions help wanted [up-for-grabs] Good issue for external contributors os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Projects
None yet
7 participants