From 0a5e220231f1dde15600b5044fb0276df9f47153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 31 Jan 2022 15:54:25 +0100 Subject: [PATCH] Ignore EPIPE errors when writing to a closed pager While a gh command is writing stdout to a pager, the user may choose to close the pager program before the pager has read all the data on its standard input. In that case, the parent gh process will receive an EPIPE error, which would bubble up its error handling and cause it to print something like: write |1: broken pipe Since this was caused by an explicit user action of closing the pager, and since the user probably doesn't want to see this uninformative error, this informs our global error handling of this error and causes it to be ignored. --- cmd/gh/main.go | 5 +++++ pkg/cmd/api/api.go | 8 +------- pkg/cmd/pr/diff/diff.go | 4 ---- pkg/cmd/repo/view/view.go | 8 +------- pkg/iostreams/epipe_other.go | 13 +++++++++++++ pkg/iostreams/epipe_windows.go | 11 +++++++++++ pkg/iostreams/iostreams.go | 22 ++++++++++++++++++++-- 7 files changed, 51 insertions(+), 20 deletions(-) create mode 100644 pkg/iostreams/epipe_other.go create mode 100644 pkg/iostreams/epipe_windows.go diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 80e9409f2..a8f1ed141 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -24,6 +24,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/factory" "github.com/cli/cli/v2/pkg/cmd/root" "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/utils" "github.com/cli/safeexec" "github.com/mattn/go-colorable" @@ -201,6 +202,7 @@ func mainRun() exitCode { rootCmd.SetArgs(expandedArgs) if cmd, err := rootCmd.ExecuteC(); err != nil { + var pagerPipeError *iostreams.ErrClosedPagerPipe if err == cmdutil.SilentError { return exitError } else if cmdutil.IsUserCancellation(err) { @@ -211,6 +213,9 @@ func mainRun() exitCode { return exitCancel } else if errors.Is(err, authError) { return exitAuth + } else if errors.As(err, &pagerPipeError) { + // ignore the error raised when piping to a closed pager + return exitOK } printError(stderr, err, cmd, hasDebug) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 34bde4c80..4762ac77d 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -3,7 +3,6 @@ package api import ( "bytes" "encoding/json" - "errors" "fmt" "io" "io/ioutil" @@ -13,7 +12,6 @@ import ( "sort" "strconv" "strings" - "syscall" "time" "github.com/MakeNowJust/heredoc" @@ -384,11 +382,7 @@ func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream _, err = io.Copy(opts.IO.Out, responseBody) } if err != nil { - if errors.Is(err, syscall.EPIPE) { - err = nil - } else { - return - } + return } if serverError == "" && resp.StatusCode > 299 { diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index dc0602da8..6e166ebb0 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -7,7 +7,6 @@ import ( "io" "net/http" "strings" - "syscall" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -112,9 +111,6 @@ func diffRun(opts *DiffOptions) error { if !opts.UseColor { _, err = io.Copy(opts.IO.Out, diff) - if errors.Is(err, syscall.EPIPE) { - return nil - } return err } diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index cb3c19fa2..e799306f0 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -6,7 +6,6 @@ import ( "net/http" "net/url" "strings" - "syscall" "text/template" "github.com/MakeNowJust/heredoc" @@ -213,12 +212,7 @@ func viewRun(opts *ViewOptions) error { View: cs.Gray(fmt.Sprintf("View this repository on GitHub: %s", openURL)), } - err = tmpl.Execute(stdout, repoData) - if err != nil && !errors.Is(err, syscall.EPIPE) { - return err - } - - return nil + return tmpl.Execute(stdout, repoData) } func isMarkdownFile(filename string) bool { diff --git a/pkg/iostreams/epipe_other.go b/pkg/iostreams/epipe_other.go new file mode 100644 index 000000000..a8a4e0476 --- /dev/null +++ b/pkg/iostreams/epipe_other.go @@ -0,0 +1,13 @@ +//go:build !windows +// +build !windows + +package iostreams + +import ( + "errors" + "syscall" +) + +func isEpipeError(err error) bool { + return errors.Is(err, syscall.EPIPE) +} diff --git a/pkg/iostreams/epipe_windows.go b/pkg/iostreams/epipe_windows.go new file mode 100644 index 000000000..69815cb15 --- /dev/null +++ b/pkg/iostreams/epipe_windows.go @@ -0,0 +1,11 @@ +package iostreams + +import ( + "errors" + "syscall" +) + +func isEpipeError(err error) bool { + // 232 is Windows error code ERROR_NO_DATA, "The pipe is being closed". + return errors.Is(err, syscall.Errno(232)) +} diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index cf7a0b04d..315248813 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -24,6 +24,11 @@ import ( const DefaultWidth = 80 +// ErrClosedPagerPipe is the error returned when writing to a pager that has been closed. +type ErrClosedPagerPipe struct { + error +} + type IOStreams struct { In io.ReadCloser Out io.Writer @@ -197,7 +202,7 @@ func (s *IOStreams) StartPager() error { if err != nil { return err } - s.Out = pagedOut + s.Out = &pagerWriter{pagedOut} err = pagerCmd.Start() if err != nil { return err @@ -211,7 +216,7 @@ func (s *IOStreams) StopPager() { return } - _ = s.Out.(io.ReadCloser).Close() + _ = s.Out.(io.WriteCloser).Close() _, _ = s.pagerProcess.Wait() s.pagerProcess = nil } @@ -430,3 +435,16 @@ func terminalSize(w io.Writer) (int, int, error) { } return 0, 0, fmt.Errorf("%v is not a file", w) } + +// pagerWriter implements a WriteCloser that wraps all EPIPE errors in an ErrClosedPagerPipe type. +type pagerWriter struct { + io.WriteCloser +} + +func (w *pagerWriter) Write(d []byte) (int, error) { + n, err := w.WriteCloser.Write(d) + if err != nil && (errors.Is(err, io.ErrClosedPipe) || isEpipeError(err)) { + return n, &ErrClosedPagerPipe{err} + } + return n, err +}