net: copy from Unix socket to os.Stdout fails with "waiting for unsupported file type"

[ianlancetaylor]

On HEAD this test function fails:

func TestCopyUnixToStdout(t *testing.T) {
	sockPath := filepath.Join(t.TempDir(), "unixsock")
	addr, err := net.ResolveUnixAddr("unix", sockPath)
	if err != nil {
		t.Fatal(err)
	}

	ln, err := net.ListenUnix("unix", addr)
	if err != nil {
		t.Fatal(err)
	}
	defer ln.Close()

	go func() {
		server, err := ln.Accept()
		if err != nil {
			t.Error(err)
			return
		}
		defer func() {
			if err := server.Close(); err != nil {
				t.Error(err)
			}
		}()
		for i := 0; i < 100; i++ {
			if _, err := server.Write(bytes.Repeat([]byte{'a'}, 1024)); err != nil {
				t.Error(err)
				return
			}
		}
	}()

	conn, err := net.Dial(ln.Addr().Network(), ln.Addr().String())
	if err != nil {
		t.Fatal(err)
	}

	if _, err := io.Copy(os.Stdout, conn); err != nil {
		t.Fatal(err)
	}
}

I see

foo_test.go:50: write /dev/stdout: waiting for unsupported file type

Here line 50 is the t.Fatal called when io.Copy fails.

This test passes with Go 1.20.

This is most likely due to https://go.dev/cl/466015. CC @panjf2000

[ianlancetaylor]

Actually it looks like this can happen with TCP as well, so it may not be CL 466015.

[gopherbot]

Change https://go.dev/cl/476335 mentions this issue: os, internal/poll: don't use splice with tty

[nightlyone]

I don't understand why there is a stat call needed asking the inode caches to achieve this instead of the much cheaper and more obviously correct normal tty check that only checks for presence a kernel file operation (equivalent to a static itab compare in Go). See e.g. https://github.com/mattn/go-isatty how to achieve this.

Checking for terminal only would still allow splicing to high speed device drivers that implement it correctly.

[bcmills]

Honestly, we've accumulated so many special-case checks on that path by now that I wonder if it would be more efficient to just try the splice call and fall back if it fails.

[ianlancetaylor]

@bcmills The problem in this particular case is that we actually use two splice calls, one from the source to a pipe, and one from the pipe to the destination. We do already try the first splice call and give up if it fails. But if it works then we have started reading from the source, so if the second splice fails then we are stuck. This change is trying to detect whether the second splice will work.

Though now that I write that it occurs to me that perhaps we could add still more complexity: if the second splice fails, we could just copy the data ourselves. The result would likely be slower overall but it might work.

@nightlyone Thanks for the suggestion.

[ianlancetaylor]

I now think that the stat call was a mistake on my part, and it may not be required at all. Taking it back out. Thanks for raising the issue.

[gopherbot]

Change https://go.dev/cl/477035 mentions this issue: os: don't check for TTY before calling splice

[andybalholm]

@ianlancetaylor

Was removing spliceNonblock a mistake?

We've been running into an issue on a couple of our proxy servers (running github.com/andybalholm/redwood) where certain downloads time out instead of completing normally. We can't reproduce the issue on other machines, and the effects aren't even quite the same on the two machines. But it's quite consistent on those machines.

So we did a git bisect, and the commit associated with this issue is where the problem starts. The problem goes away if we revert the part of the change that relates to spliceNonblock:

diff --git a/src/internal/poll/splice_linux.go b/src/internal/poll/splice_linux.go
index 9505c5dcfc..4690962ae6 100644
--- a/src/internal/poll/splice_linux.go
+++ b/src/internal/poll/splice_linux.go
@@ -13,6 +13,9 @@ import (
 )
 
 const (
+       // spliceNonblock makes calls to splice(2) non-blocking.
+       spliceNonblock = 0x2
+
        // maxSpliceSize is the maximum amount of data Splice asks
        // the kernel to move in a single call to splice(2).
        // We use 1MB as Splice writes data through a pipe, and 1MB is the default maximum pipe buffer size,
@@ -89,7 +92,7 @@ func spliceDrain(pipefd int, sock *FD, max int) (int, error) {
                return 0, err
        }
        for {
-               n, err := splice(pipefd, sock.Sysfd, max, 0)
+               n, err := splice(pipefd, sock.Sysfd, max, spliceNonblock)
                if err == syscall.EINTR {
                        continue
                }
@@ -127,7 +130,7 @@ func splicePump(sock *FD, pipefd int, inPipe int) (int, error) {
        }
        written := 0
        for inPipe > 0 {
-               n, err := splice(sock.Sysfd, pipefd, inPipe, 0)
+               n, err := splice(sock.Sysfd, pipefd, inPipe, spliceNonblock)
                // Here, the condition n == 0 && err == nil should never be
                // observed, since Splice controls the write side of the pipe.
                if n > 0 {

When we make this change to the tip of the master branch, our problem goes away, and all tests still pass.

Interestingly, the removal of spliceNonblock is not mentioned in the commit message. So maybe it was not really intended…?

[ianlancetaylor]

I removed spliceNonblock because it didn't seem to be serving any purpose, and because all the tests passed even when it was removed. I still don't see any purpose for it. The descriptors are expected to already be in non-blocking mode anyhow.

Is there any difference in the kernel version on the machines where your code fails?

Can you share a test case that demonstrates the failure? Thanks.