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

os: don't waste extra threads for opening files #32618

Open
Al2Klimov opened this issue Jun 14, 2019 · 12 comments
Open

os: don't waste extra threads for opening files #32618

Al2Klimov opened this issue Jun 14, 2019 · 12 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Al2Klimov
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.12.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/aklimov/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/aklimov/Git/golang"
GOPROXY=""
GORACE=""
GOROOT="/Users/aklimov/Git/brew/Cellar/go/1.12.6/libexec"
GOTMPDIR=""
GOTOOLDIR="/Users/aklimov/Git/brew/Cellar/go/1.12.6/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3h/0nn80rp973b08bd0r56g5pd80000gn/T/go-build873614728=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

  1. I've built the program shown below,
  2. ran mkfifo pipe and
  3. ran GOMAXPROCS=1 lldb theprogramshownbelow.
package main

import (
	"fmt"
	"os"
	"runtime"
	"time"
)

func main() {
	for i := runtime.NumCPU() * 2; i > 0; i-- {
		go openPipe()
	}

	for {
		time.Sleep(time.Second)
	}
}

func openPipe() {
	p, errOpen := os.Open("./pipe")
	fmt.Printf("os.Open(\"./pipe\"): %#v\n", errOpen)
	p.Close()

	for {
		time.Sleep(time.Second)
	}
}

What did you expect to see?

Not much more threads than one.

What did you see instead?

11 threads including a lot of hanging in the open(2) syscall.

@gopherbot
Copy link

Change https://golang.org/cl/182397 mentions this issue: os: open(2) files w/o blocking

@FiloSottile FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 14, 2019
@FiloSottile FiloSottile added this to the Unplanned milestone Jun 14, 2019
@FiloSottile
Copy link
Contributor

/cc @rsc @ianlancetaylor

@tv42
Copy link

tv42 commented Jun 21, 2019

After the suggested change, os.Open("/dev/stdout") and such will likely cause explosions in other processes. Unfortunately nonblocking on Unix is a property of the file, not the file descriptor (or a separate syscall), and making unsuspecting processes suddenly have their shared files be nonblocking is not a good idea.

@Al2Klimov
Copy link
Author

Are you sure? fcntl(2) being effectively used in #32619 "provides for control over descriptors" according to the man page.

@tv42
Copy link

tv42 commented Jun 21, 2019

@Al2Klimov The per-FD flags are accessed with F_GETFD/F_SETFD, the per-file flags are accessed with F_GETFL/F_SETFL. O_NONBLOCK is defined as a "file status flag" (not file descriptor flag), and used with F_SETFL.

@ianlancetaylor
Copy link
Contributor

If /dev/stdout is connected to a pipe, then I suspect that we may already cause this kind of confusion, since in that case we will set the O_NONBLOCK flag on the file descriptor via syscall.SetNonblock. That is, I'm not sure that the suggested change introduces a problem for /dev/stdout that does not already exist.

@tv42
Copy link

tv42 commented Jun 21, 2019

@ianlancetaylor Unf actually that is pretty nasty. I mean, one could hope that in 2019 every process was already prepared to handle EAGAIN, but it really wouldn't surprise me if doing that still either made a bunch of things error out, or sent them into busy loops.

@tv42
Copy link

tv42 commented Jun 21, 2019

Fun fact: a Go process reading stdin is the only process on my machine with a non-blocking stdin/stdout.

$ grep -h ^flags: /proc/*/fdinfo/{0,1,2}|sort|uniq -c|sort -n
      2 flags:	02000000                   <----------- this is non-blocking
     11 flags:	00
     11 flags:	0100002
     15 flags:	01
    117 flags:	0100000
    153 flags:	02
    177 flags:	0100001

@tv42
Copy link

tv42 commented Jun 22, 2019

So my earlier argument was "some things will break", with a vague promise of breaking. Here's a concrete breakage if you automatically add O_NONBLOCK to all os.OpenFile calls:

man 7 fifo

  A process can open a FIFO in nonblocking mode.  In this  case,  opening
  for  read-only succeeds even if no one has opened on the write side yet
  and opening for write-only fails with ENXIO (no such device or address)
  unless the other end has already been opened.

So, automatic O_NONBLOCK at open(2) time will break all fifo uses where the write side is opened first.

@Al2Klimov
Copy link
Author

Strange. With Create() instead of Open() I also got error=nil.

@dominikh
Copy link
Member

I'd like to point out that O_NONBLOCK is an attribute of the file description (where description != descriptor), not the file. That is, two calls to open will produce two different file descriptions. A file description can have many file descriptors. For example, calling dup on a file descriptor will yield a new file descriptor that shares the first file descriptor's file description. Same goes for fork.

The core issues explained by @tv42 still apply, but two processes each opening the same file cannot screw each other up, as they will not share the same file description.

Of course, the file descriptors 0–2 still commonly share a single file description, but opening /dev/stdout in non-blocking mode should be safe.

Refer to the following excerpt of open(2)

   A call to open() creates a new open file description, an entry in the
   system-wide table of open files.  The open file description records
   the file offset and the file status flags (see below). 

as well as the entire subsection Open file descriptions of section NOTES.

@tv42
Copy link

tv42 commented Jun 22, 2019

@dominikh is correct, I misremembered the details. The traps only activate if you share an open fd (inherit it or pass it over unix domain socket). So by that logic, just os.OpenFile("/dev/stdout", os.O_WRONLY|syscall.O_NONBLOCK, 0) shouldn't hurt others.

Adding O_NONBLOCK to all opens will still break fifo write-side opening with no readers:

package main

import (
	"log"
	"os"
	"syscall"
)

func main() {
	if err := os.Remove("fifo"); err != nil && !os.IsNotExist(err) {
		log.Fatalf("remove: %v", err)
	}
	if err := syscall.Mkfifo("fifo", 0644); err != nil {
		log.Fatalf("mkfifo: %v", err)
	}

	f, err := os.OpenFile("fifo", os.O_WRONLY|syscall.O_NONBLOCK, 0644)
	if err != nil {
		log.Fatalf("open: %v", err)
	}
	defer f.Close()
}
$ go run .
2019/06/22 12:00:39 open: open fifo: no such device or address
exit status 1

Also, I'd like to clarify the issue subject: it should be "os: don't waste extra threads when opening fifos". This bug is only relevant if you're opening lots of FIFOs for reading concurrently. That's a really rare edge case scenario. The long wait in the syscall doesn't trigger on most kinds of files.

FIFOs are a rare special case, and 1) leaving O_NONBLOCK out wastes system call threads 2) adding O_NONBLOCK breaks some use cases.

POSIX doesn't define what O_NONBLOCK means for FIFOs, any improvement here is Linux-only.

You could perhaps argue for automatic O_NONBLOCK as long as it's the file flag is O_RDONLY. But to deal with fifos, you're making raw syscalls anyway to create one, etc. I'd be ready to call this an obscure enough an edge case that making the caller use syscalls and os.NewFile is not too much of a burden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants