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, runtime: should not have to wait for 10 EPIPE failures before raising SIGPIPE #11845

Closed
ianlancetaylor opened this issue Jul 23, 2015 · 13 comments
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

Back in https://go.googlesource.com/go/+/f7d3eb9db97a65a43b8d6b8bf42b8698fe4468ee we added code so that if we saw 10 EPIPE failures in a row on the same file descriptor, we would exit the program. Today we exit the program by raising a SIGPIPE signal, but we still wait for 10 failures. That leads to issues like https://golang.org/issue/11839 and workarounds like https://golang.org/cl/12571.

Now that we have the os/signal package, Go programs have the mechanisms they need to control this. I propose that we change the Go runtime to not handle SIGPIPE by default. If os/signal.Notify is used for SIGPIPE, that only at that point should we add a handler for SIGPIPE. Then we should remove the EPIPE handling from the os package, and treat it like any other error.

@ianlancetaylor ianlancetaylor self-assigned this Jul 23, 2015
@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jul 23, 2015
@robpike
Copy link
Contributor

robpike commented Jul 27, 2015

I think this proposal is a bad idea. Any Go program whose output might go into a pipe will now need a signal handler. The flag in 11389 is panicking on error, not EPIPE per se.

@ianlancetaylor
Copy link
Contributor Author

This proposal would make Go programs act like C programs by default: writing to a closed pipe would cause the program to exit due to a SIGPIPE signal. To avoid that, a Go program would have to call signal.Ignore(syscall.SIGPIPE), which I agree is not ideal because it's Unix-specific.

I also agree that this might be a bad default for Go. In that case, I think we need a simple way for Go programs to say that they should exit when writing to a closed pipe, to avoid the contortions required to check for EPIPE. It would be appropriate for any Go program that acts as a command line client to make that call. Right now, a Go program can do it by calling syscall.Reset(syscall.SIGPIPE), and perhaps the go tool should do that at least for the help command. But, again, it's Unix-specific.

Maybe we need something like runtime.ExitOnBrokenPipe(bool).

@rsc
Copy link
Contributor

rsc commented Jul 27, 2015

FWIW I disagree that Go programs should act like C programs by default, especially because you get SIGPIPE for writes to closed network connections too. If we could somehow make SIGPIPE only come in for writes to standard output or standard error, maybe that would be okay.

I think the real problem is that the template package does not distinguish template errors from write errors in any useful way.

@ianlancetaylor
Copy link
Contributor Author

There may be a problem with the template package, but I think there is also a different problem here. If I have a command line tool that generates output, I want that tool to receive a SIGPIPE signal, and exit because of it, if it is writing to a closed pipe. Having that only happen after 10 writes fail is arbitrary and fails if I do fewer than 10 writes.

@mgumz
Copy link

mgumz commented Jul 27, 2015

one might also have a long-running go-program which is supervised by systemd. and it is "logging" to stdout/stderr. and systemd will write that log to journald. and someone is restarting journald (because that someone is changing, lets say, the compression of the .journal files). and that someone is running systemd-218 or below (so systemd won't save the given filedescriptors such as implemented in systemd-219+).

upon journald-restart the program will get SIGPIPE and and it is very reasonable to shut down the program as well (since someone removed the "logging" infrastructure underneath).

@robpike
Copy link
Contributor

robpike commented Jul 28, 2015

See #11898

@ianlancetaylor
Copy link
Contributor Author

Proposal after talking to Russ:

  1. Drop the EPIPE counter.
  2. By default, if we get an EPIPE on os.Stdout or os.Stderr, change the SIGPIPE handler to SIG_DFL and raise SIGPIPE (that is, do what we currently do after getting 10 EPIPE errors, but do it immediately.)
  3. On other descriptors, simply return EPIPE to the caller of Write (as we do today, up to 10 EPIPE errors).
  4. If the program calls os/signal.Notify(syscall.SIGPIPE) or os/signal.Ignore(syscall.SIGPIPE), then do not handle EPIPE specially on os.Stdout/os.Stderr.

With these rules:

  1. A command line program writing to a pipe on Stdout or Stderr will do the right thing by default.
  2. A server writing to a closed socket will do the right thing by default (i.e., get an error but not crash).
  3. The program may look for an EPIPE error as it can today.
  4. The program may call os/signal.Notify to explicitly handle SIGPIPE in some other way.

Of course, this should all be documented somewhere.

@rsc
Copy link
Contributor

rsc commented Nov 23, 2015

"Proposal" accepted.

@mdempsky
Copy link
Contributor

mdempsky commented Dec 8, 2015

Sorry to be pedantic, but what exactly does it mean to get an EPIPE "on os.Stdout or os.Stderr"? E.g.,

  1. If I re-set os.Stdout = os.NewFile(1, "/dev/stdout") and then get an EPIPE writing to os.Stdout, is SIGPIPE raised?
  2. What if I set os.Stdout to an *os.File value created some other way and get EPIPE?
  3. What if I do stdout := os.Stdout; os.Stdout = nil; stdout.Write(...) and get an EPIPE?
  4. What if I just do os.NewFile(1, "/dev/stdout").Write(...) and get an EPIPE?

I'm thinking only case 3 should raise SIGPIPE (i.e., the behavior is tied to the *os.File values used to initialize os.Stdout and os.Stderr).

@rsc
Copy link
Contributor

rsc commented Dec 8, 2015

I was thinking it meant fd 1 or fd 2.

@mdempsky
Copy link
Contributor

mdempsky commented Dec 8, 2015

That seems justifiable to me too.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/18151 mentions this issue.

@mikioh mikioh reopened this Jan 5, 2016
@mikioh mikioh closed this as completed Jan 5, 2016
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/18226 mentions this issue.

ianlancetaylor added a commit that referenced this issue Jan 6, 2016
Update #11845.

Change-Id: I1c248dc48abc62e51836b9ba50d6deb89706c730
Reviewed-on: https://go-review.googlesource.com/18226
Reviewed-by: Russ Cox <rsc@golang.org>
@golang golang locked and limited conversation to collaborators Jan 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants