GetSocketOption/SetSocketOption should not close underlying socket

[Dweeberly]

Description

Assume that _socket is an open (active) TCP socket. The following call will throw an exception and close the socket

Socket _socket;
_socket.NoDelay = true;
_socket.Blocking = true;
_socket.ReceiveTimeout = -1;
_socket.SendTimeout = -1;
...
IPAddress ipaddr = <setup your server ip>;
IPEndPoint svrEndPoint = new IPEndPoint(ipaddr, _serverPort);
var listener = new Socket(ipaddr.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
listener.Bind(svrEndPoint);
listener.Listen(2);
_socket = listener.Accept();
_socket.GetSocketOption(SocketOptionLevel.Socket, SocketOptionName.DontLinger)

This option (DontLinger) isn't supported on Linux so the exception is appropriate, but closing the socket isn't. To quote a friend this "violates the principle of least astonishment".

Configuration

Version: .NET core 5.0
OS: Linux Ubuntu 20.04.lts
Arch: x64
Configuration: The option isn't supported on this Linux distro but is on Windows. My code caught the exception but did not expect the socket to be closed.

Regression?

I do not believe this is a regression.

Other information

Here is what I see in the codebase.
in /src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
The GetSocketOption methods call into the OS:

...
SocketError errorCode = SocketPal.GetSockOpt(
    _handle,
    optionLevel,
    optionName,
    out optionValue);
...
// Throw an appropriate SocketException if the native call fails.
if (errorCode != SocketError.Success)
{
    UpdateStatusAfterSocketErrorAndThrowException(errorCode);
}

So if the call returns a not supported error, UpdateStatusAfterSocketErrorAndThrowExecption is called.
That calls an internal method named UpdateStatusAfterSocketError which:

if (_isConnected && (_handle.IsInvalid || (errorCode != SocketError.WouldBlock &&
        errorCode != SocketError.IOPending && errorCode != SocketError.NoBufferSpaceAvailable &&
        errorCode != SocketError.TimedOut)))
{
    // The socket is no longer a valid socket.
    if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Invalidating socket.");
    SetToDisconnected();
}

Honestly, that looks correct to me, but perhaps the errorCode isn't what I would be expecting for an unsupported option name, or the handled has somehow (?) become invalid. I wasn't able to look at this in the debugger, so I can't say what the conditions were or if this is even the right spot (but I don't see anywhere else)

A few nights ago I was down in some mono code and that clearly would have created this effect. It check the options first and if they failed an exception was thrown (never calling the OS). I assume that code is no longer used in the v5 .net runtime.

Workaround is the check the Linger option.

[ghost]

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

Issue Details

---

Description

Assume that _socket is an open (active) TCP socket. The following call will throw an exception and close the socket

Socket _socket;
_socket.NoDelay = true;
_socket.Blocking = true;
_socket.ReceiveTimeout = -1;
_socket.SendTimeout = -1;
...
IPAddress ipaddr = <setup your server ip>;
IPEndPoint svrEndPoint = new IPEndPoint(ipaddr, _serverPort);
var listener = new Socket(ipaddr.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
listener.Bind(svrEndPoint);
listener.Listen(2);
_socket = listener.Accept();
_socket.GetSocketOption(SocketOptionLevel.Socket, SocketOptionName.DontLinger)

This option (DontLinger) isn't supported on Linux so the exception is appropriate, but closing the socket isn't. To quote a friend this "violates the principle of least astonishment".

Configuration

Version: .NET core 5.0
OS: Linux Ubuntu 20.04.lts
Arch: x64
Configuration: The option isn't supported on this Linux distro but is on Windows. My code caught the exception but did not expect the socket to be closed.

Regression?

I do not believe this is a regression.

Other information

Here is what I see in the codebase.
in /src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
The GetSocketOption methods call into the OS:

...
            SocketError errorCode = SocketPal.GetSockOpt(
                _handle,
                optionLevel,
                optionName,
                out optionValue);
...
            // Throw an appropriate SocketException if the native call fails.
            if (errorCode != SocketError.Success)
            {
                UpdateStatusAfterSocketErrorAndThrowException(errorCode);
            }

So if the call returns a not supported error, UpdateStatusAfterSocketErrorAndThrowExecption is called.
That calls an internal method named UpdateStatusAfterSocketError which:

            if (_isConnected && (_handle.IsInvalid || (errorCode != SocketError.WouldBlock &&
                    errorCode != SocketError.IOPending && errorCode != SocketError.NoBufferSpaceAvailable &&
                    errorCode != SocketError.TimedOut)))
            {
                // The socket is no longer a valid socket.
                if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Invalidating socket.");
                SetToDisconnected();
            }

Honestly, that looks correct to me, but perhaps the errorCode isn't what I would be expecting for an unsupported option name, or the handled has somehow (?) become invalid. I wasn't able to look at this in the debugger, so I can't say what the conditions were or if this is even the right spot (but I don't see anywhere else)

A few nights ago I was down in some mono code and that clearly would have created this effect. It check the options first and if they failed an exception was thrown (never calling the OS). I assume that code is no longer used in the v5 .net runtime.

Author:	Dweeberly
Assignees:	-
Labels:	area-System.Net.Sockets, untriaged
Milestone:	-

[antonfirsov]

@tmds @wfurt would it make sense to implement SocketOptionName.DontLinger on top of SO_LINGER so that DontLinger controls the value of linger.l_onoff? Or at least make it gettable?

To me it looks like this is what it does on Windows:
https://docs.microsoft.com/en-us/windows/win32/winsock/sol-socket-socket-options

[tmds]

@tmds @wfurt would it make sense to implement SocketOptionName.DontLinger on top of SO_LINGER so that DontLinger controls the value of linger.l_onoff? Or at least make it gettable?

The issue with the setter is that we need to pick a timeout. And a getter is not so useful, this is an option a user will want to set.

SocketOptionName.Linger can be used instead.

We should consider to change Get/SetSocketOption to still throw an exception for unsupported options and not close the socket.

[wfurt]

I would think we can pick reasonable default for timeout oof needed. But aside from that I would agree that failing to set options should not close the underlying socket.

[karelz]

Triage: We agree we should not close underlying sockets when set options fails. Retitling to track it here.