Revert "Use socketpair to implement Process.Start redirection (#34861)"

[eiriktsarpalis]

Reverts commit c44dc40, introduced by #34861.

Fixes #40727.

Please don't merge while I test the changes in WSL1.

[ghost]

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

[stephentoub]

If we go ahead with this reverting, we need to have an issue to redo it in .NET 6.0. This was fixing a real issue.
cc: @davidfowl

[stephentoub]

(I'd also love it if we could do a bit more debugging and see whether there's a simple fix that would enable this to work in WSL1, such as being a bit more forgiving of various error conditions.)

[adamsitnik]

How about not reverting it but adding a fallback based on whether to socket is connected or not?

private static Stream OpenStream(int fd, FileAccess access)
{
    var socket = new Socket(new SafeSocketHandle((IntPtr)fd, ownsHandle: true));
    if (socket.IsConnected)
    {
        return new NetworkStream(socket, access, ownsSocket: true);
    }
    else //WSL1
    {
        return new FileStream(
            new SafeFileHandle((IntPtr)fd, ownsHandle: true),
            access, StreamBufferSize, isAsync: false);
    }
}

@stephentoub @eiriktsarpalis what do you think?

[stephentoub]

Thanks, Adam. My preference would be to actually fix the Socket ctor; both reverting this and working around it in Process will still leave the ctor buggy in WSL1 for any other consumption. But short of that, yes, I'd prefer a (temporary) workaround in Process rather than reverting. (But note a workaround like above would be a little more complicated, e.g. the FileStream would need to keep the Socket alive.)

[eiriktsarpalis]

Do we have reliable platform detection for WSL1? We could just use that to switch to the old implementation.

[adamsitnik]

My preference would be to actually fix the Socket ctor

same here

Do we have reliable platform detection for WSL1

The problem is that it can also occur on other Unixes, like FreeBSD here: #33051

[eiriktsarpalis]

My preference would be to actually fix the Socket ctor

I don't believe this is really possible at the socket layer for WSL1. We'd need to somehow infer the address family and there's a lot of other weird behaviour going on, for instance calling getpeername on a socket pair will sometimes return ENOTCONN. Our safest bet is addressing this at the process layer.

For the fallback logic, it might make sense to do some form of platform detection for WSL1. The best known approach is reading the /proc/version file, honestly though this is a heurestic I'm not confident putting in production code.

cc @wfurt

[stephentoub]

@eiriktsarpalis, did you debug into

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Line 2486 in 6072e4d

int32_t SystemNative_GetSocketType(intptr_t socket, int32_t* addressFamily, int32_t* socketType, int32_t* protocolType, int32_t* isListening)

to see where things were going awry?

[eiriktsarpalis]

Sure, it looks like the getsockopt call in line 2522 is returning a non-zero value. The same seems to happen in line 2532 which results in an unknown socket type being returned.

[stephentoub]

Presumably -1. Can you see what errno is?

[eiriktsarpalis]

It's EINVAL.

[stephentoub]

It's EINVAL.

Thanks. I suspect the right fix is to be more forgiving of such errors, as we do later in the method:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

Lines 220 to 226 in 12f8e34

	case SocketError.InvalidArgument:
	// On some OSes (e.g. macOS), EINVAL means the socket has been shut down.
	// This can happen if, for example, socketpair was used and the parent
	// process closed its copy of the child's socket. Since we don't know
	// whether we're actually connected or not, err on the side of saying
	// we're connected.
	_isConnected = true;

[eiriktsarpalis]

Closing in favor of #40851