syscall,internal/poll: move pipe check from syscall.Seek to callers

On Windows, syscall.Seek is a thin wrapper over SetFilePointerEx [1],
which does not work on pipes, although it doesn't return an error on
that case. To avoid this undefined behavior, Seek defensively
calls GetFileType and errors if the type is FILE_TYPE_PIPE.

The problem with this approach is that Seek is a low level
foundational function that can be called many times for the same file,
and the additional cgo call (GetFileType) will artificially slow
down seek operations. I've seen GetFileType to account for 10% of cpu
time in seek-intensive workloads.

A better approach, implemented in this CL, would be to move the check
one level up, where many times the file type is already known so the
GetFileType is unnecessary.

The drawback is that syscall.Seek has had this behavior since pipes
where first introduced to Windows in
https://codereview.appspot.com/1715046 and someone could be relying on
it. On the other hand, this behavior is not documented, so we couldn't
be breaking any contract.

[1] https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-setfilepointerex

Change-Id: I7602182f9d08632e22a8a1635bc8ad9ad35a5056
Reviewed-on: https://go-review.googlesource.com/c/go/+/493626
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>

diff --git a/src/internal/poll/fd_windows.go b/src/internal/poll/fd_windows.go
index f863ecb99829b776c1788c2126e44579409e92f7..9df39edced5bba986e7e478f1b9b9734116a8cba 100644 (file)
--- a/src/internal/poll/fd_windows.go
+++ b/src/internal/poll/fd_windows.go
@@ -522,6 +522,10 @@ func (fd *FD) readConsole(b []byte) (int, error) {
 
 // Pread emulates the Unix pread system call.
 func (fd *FD) Pread(b []byte, off int64) (int, error) {
+       if fd.kind == kindPipe {
+               // Pread does not work with pipes
+               return 0, syscall.ESPIPE
+       }
        // Call incref, not readLock, because since pread specifies the
        // offset it is independent from other reads.
        if err := fd.incref(); err != nil {
@@ -744,6 +748,10 @@ func (fd *FD) writeConsole(b []byte) (int, error) {
 
 // Pwrite emulates the Unix pwrite system call.
 func (fd *FD) Pwrite(buf []byte, off int64) (int, error) {
+       if fd.kind == kindPipe {
+               // Pwrite does not work with pipes
+               return 0, syscall.ESPIPE
+       }
        // Call incref, not writeLock, because since pwrite specifies the
        // offset it is independent from other writes.
        if err := fd.incref(); err != nil {
@@ -992,6 +1000,9 @@ func (fd *FD) Accept(sysSocket func() (syscall.Handle, error)) (syscall.Handle,
 
 // Seek wraps syscall.Seek.
 func (fd *FD) Seek(offset int64, whence int) (int64, error) {
+       if fd.kind == kindPipe {
+               return 0, syscall.ESPIPE
+       }
        if err := fd.incref(); err != nil {
                return 0, err
        }

diff --git a/src/internal/poll/sendfile_windows.go b/src/internal/poll/sendfile_windows.go
index 50c3ee86c0fc0565da9244797a96ac6f2b3af860..8c3353bc6ffa793d93a721a573c7a8672884857e 100644 (file)
--- a/src/internal/poll/sendfile_windows.go
+++ b/src/internal/poll/sendfile_windows.go
@@ -15,6 +15,9 @@ func SendFile(fd *FD, src syscall.Handle, n int64) (written int64, err error) {
                // TransmitFile does not work with pipes
                return 0, syscall.ESPIPE
        }
+       if ft, _ := syscall.GetFileType(src); ft == syscall.FILE_TYPE_PIPE {
+               return 0, syscall.ESPIPE
+       }
 
        if err := fd.writeLock(); err != nil {
                return 0, err

diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go
index 8687d1cc21fba3113f580ff7536be7dffee52396..cf6049a2f2a3cf39fd626c51ac5e076602942e85 100644 (file)
--- a/src/syscall/syscall_windows.go
+++ b/src/syscall/syscall_windows.go
@@ -498,11 +498,6 @@ func Seek(fd Handle, offset int64, whence int) (newoffset int64, err error) {
        case 2:
                w = FILE_END
        }
-       // use GetFileType to check pipe, pipe can't do seek
-       ft, _ := GetFileType(fd)
-       if ft == FILE_TYPE_PIPE {
-               return 0, ESPIPE
-       }
        err = setFilePointerEx(fd, offset, &newoffset, w)
        return
 }