From f2dd5689de0a4e10fcf185a5eb3d1b25d927117e Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Mon, 18 Feb 2019 21:40:01 -0800 Subject: [PATCH 1/4] executor: simplify error handling * Don't catch panics. We mostly had this to catch issues with the _commands_ library but catching random panics is really not the best idea. Worse, this code was rather arbitrary and only caught panics implementing `error`. * Make sure to wait for the PostRun to finish before returning. --- executor.go | 71 +++++++++++------------------------------------------ 1 file changed, 15 insertions(+), 56 deletions(-) diff --git a/executor.go b/executor.go index d9df7819..05c43973 100644 --- a/executor.go +++ b/executor.go @@ -2,7 +2,6 @@ package cmds import ( "context" - "runtime/debug" ) type Executor interface { @@ -37,14 +36,14 @@ type executor struct { root *Command } -func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) (err error) { +func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) error { cmd := req.Command if cmd.Run == nil { return ErrNotCallable } - err = cmd.CheckArguments(req) + err := cmd.CheckArguments(req) if err != nil { return err } @@ -58,7 +57,6 @@ func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) (e // contains the error returned by PostRun errCh := make(chan error, 1) - if cmd.PostRun != nil { if typer, ok := re.(interface { Type() PostRunType @@ -71,22 +69,8 @@ func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) (e re, res = NewChanResponsePair(req) go func() { - var closeErr error - defer close(errCh) - - err := cmd.PostRun[typer.Type()](res, lower) - closeErr = lower.CloseWithError(err) - if closeErr == ErrClosingClosedEmitter { - // ignore double close errors - closeErr = nil - } - errCh <- closeErr - - if closeErr != nil && err != nil { - log.Errorf("error closing connection: %s", closeErr) - log.Errorf("close caused by error: %s", err) - } + errCh <- lower.CloseWithError(cmd.PostRun[typer.Type()](res, lower)) }() } } else { @@ -94,42 +78,17 @@ func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) (e close(errCh) } - defer func() { - // catch panics in Run (esp. from re.SetError) - if v := recover(); v != nil { - log.Errorf("panic in command handler at %s", debug.Stack()) - - // if they are errors - if err, ok := v.(error); ok { - // use them as return error - closeErr := re.CloseWithError(err) - if closeErr == ErrClosingClosedEmitter { - // ignore double close errors - closeErr = nil - } else if closeErr != nil { - log.Errorf("error closing connection: %s", closeErr) - if err != nil { - log.Errorf("close caused by error: %s", err) - } - } - } else { - // otherwise keep panicking. - panic(v) - } - - // wait for PostRun to finish - <-errCh - } - }() - err = cmd.Run(req, re, env) - err = re.CloseWithError(err) - if err == ErrClosingClosedEmitter { - // ignore double close errors - return nil - } else if err != nil { - return err + runCloseErr := re.CloseWithError(cmd.Run(req, re, env)) + postCloseErr := <-errCh + switch runCloseErr { + case ErrClosingClosedEmitter, nil: + default: + return runCloseErr } - - // return error from the PostRun Close - return <-errCh + switch postCloseErr { + case ErrClosingClosedEmitter, nil: + default: + return postCloseErr + } + return nil } From 6a12eea7a7a138fe87b73fbd3ab24aa7252cf8f7 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 19 Feb 2019 00:27:49 -0800 Subject: [PATCH 2/4] cli: get rid of exit channel It wasn't a reliable way to determine if the command was "done". We need to wait for run, post-run, etc. to finish first. * Fixes #143 This commit: 1. Removes the exit channel from the CLI response emitter. 2. Adds a `Status()` function to the `cli.ResponseEmitter` as a replacement for (1). 3. Renames `Exit(code)` to `SetStatus(code)`. This new function doesn't immediately _do_ anything other than set the status. 4. Make sure to wait for _everything_ to finish before returning from `cli.Run`. --- cli/responseemitter.go | 113 ++++++++++++--------------- cli/responseemitter_test.go | 8 +- cli/run.go | 31 +++----- cli/run_test.go | 64 +++++++++++++++ cli/single_test.go | 7 +- examples/adder/cmd.go | 4 +- examples/adder/local/main.go | 20 ++--- examples/adder/remote/client/main.go | 31 +++----- 8 files changed, 149 insertions(+), 129 deletions(-) create mode 100644 cli/run_test.go diff --git a/cli/responseemitter.go b/cli/responseemitter.go index decadeef..3849be7b 100644 --- a/cli/responseemitter.go +++ b/cli/responseemitter.go @@ -14,21 +14,15 @@ import ( var _ ResponseEmitter = &responseEmitter{} -func NewResponseEmitter(stdout, stderr io.Writer, req *cmds.Request) (cmds.ResponseEmitter, <-chan int, error) { - ch := make(chan int) +func NewResponseEmitter(stdout, stderr io.Writer, req *cmds.Request) (ResponseEmitter, error) { encType, enc, err := cmds.GetEncoder(req, stdout, cmds.TextNewline) - if err != nil { - close(ch) - return nil, ch, err - } return &responseEmitter{ stdout: stdout, stderr: stderr, encType: encType, enc: enc, - ch: ch, - }, ch, err + }, err } // ResponseEmitter extends cmds.ResponseEmitter to give better control over the command line @@ -37,7 +31,12 @@ type ResponseEmitter interface { Stdout() io.Writer Stderr() io.Writer - Exit(int) + + // SetStatus sets the exit status for this command. + SetStatus(int) + + // Status returns the exit status for the command. + Status() int } type responseEmitter struct { @@ -50,8 +49,6 @@ type responseEmitter struct { encType cmds.EncodingType exit int closed bool - - ch chan<- int } func (re *responseEmitter) Type() cmds.PostRunType { @@ -62,36 +59,6 @@ func (re *responseEmitter) SetLength(l uint64) { re.length = l } -func (re *responseEmitter) CloseWithError(err error) error { - var msg string - switch err { - case nil: - return re.Close() - case context.Canceled: - msg = "canceled" - case context.DeadlineExceeded: - msg = "timed out" - default: - msg = err.Error() - } - - re.l.Lock() - defer re.l.Unlock() - - if re.closed { - return cmds.ErrClosingClosedEmitter - } - - re.exit = 1 // TODO we could let err carry an exit code - - _, err = fmt.Fprintln(re.stderr, "Error:", msg) - if err != nil { - return err - } - - return re.close() -} - func (re *responseEmitter) isClosed() bool { re.l.Lock() defer re.l.Unlock() @@ -100,24 +67,39 @@ func (re *responseEmitter) isClosed() bool { } func (re *responseEmitter) Close() error { + return re.CloseWithError(nil) +} + +func (re *responseEmitter) CloseWithError(err error) error { re.l.Lock() defer re.l.Unlock() - return re.close() -} - -func (re *responseEmitter) close() error { if re.closed { return cmds.ErrClosingClosedEmitter } + re.closed = true + + var msg string + if err != nil { + if re.exit == 0 { + // Default "error" exit code. + re.exit = 1 + } + switch err { + case context.Canceled: + msg = "canceled" + case context.DeadlineExceeded: + msg = "timed out" + default: + msg = err.Error() + } - re.ch <- re.exit - close(re.ch) + fmt.Fprintln(re.stderr, "Error:", msg) + } defer func() { re.stdout = nil re.stderr = nil - re.closed = true }() // ignore error if the operating system doesn't support syncing std{out,err} @@ -131,23 +113,19 @@ func (re *responseEmitter) close() error { return false } + var errStderr, errStdout error if f, ok := re.stderr.(*os.File); ok { - err := f.Sync() - if err != nil { - if !ignoreError(err) { - return err - } - } + errStderr = f.Sync() } if f, ok := re.stdout.(*os.File); ok { - err := f.Sync() - if err != nil { - if !ignoreError(err) { - return err - } - } + errStdout = f.Sync() + } + if errStderr != nil && !ignoreError(errStderr) { + return errStderr + } + if errStdout != nil && !ignoreError(errStdout) { + return errStdout } - return nil } @@ -220,11 +198,16 @@ func (re *responseEmitter) Stdout() io.Writer { return re.stdout } -// Exit sends code to the channel that was returned by NewResponseEmitter, so main() can pass it to os.Exit() -func (re *responseEmitter) Exit(code int) { - defer re.Close() - +// SetStatus sets the exit status of the command. +func (re *responseEmitter) SetStatus(code int) { re.l.Lock() defer re.l.Unlock() re.exit = code } + +// Status _returns_ the exit status of the command. +func (re *responseEmitter) Status() int { + re.l.Lock() + defer re.l.Unlock() + return re.exit +} diff --git a/cli/responseemitter_test.go b/cli/responseemitter_test.go index 5027c841..4412f623 100644 --- a/cli/responseemitter_test.go +++ b/cli/responseemitter_test.go @@ -23,17 +23,17 @@ type tcCloseWithError struct { func (tc tcCloseWithError) Run(t *testing.T) { req := &cmds.Request{} - cmdsre, exitCh, err := NewResponseEmitter(tc.stdout, tc.stderr, req) + cmdsre, err := NewResponseEmitter(tc.stdout, tc.stderr, req) if err != nil { t.Fatal(err) } re := cmdsre.(ResponseEmitter) - go tc.f(re, t) + tc.f(re, t) - if exitCode := <-exitCh; exitCode != tc.exExit { - t.Fatalf("expected exit code %d, got %d", tc.exExit, exitCode) + if re.Status() != tc.exExit { + t.Fatalf("expected exit code %d, got %d", tc.exExit, re.Status()) } if tc.stdout.String() != tc.exStdout { diff --git a/cli/run.go b/cli/run.go index a326e463..f176de59 100644 --- a/cli/run.go +++ b/cli/run.go @@ -120,11 +120,6 @@ func Run(ctx context.Context, root *cmds.Command, return err } - var ( - re cmds.ResponseEmitter - exitCh <-chan int - ) - encTypeStr, _ := req.Options[cmds.EncLong].(string) encType := cmds.EncodingType(encTypeStr) @@ -133,23 +128,17 @@ func Run(ctx context.Context, root *cmds.Command, req.Options[cmds.EncLong] = cmds.JSON } - // first if condition checks the command's encoder map, second checks global encoder map (cmd vs. cmds) - re, exitCh, err = NewResponseEmitter(stdout, stderr, req) + re, err := NewResponseEmitter(stdout, stderr, req) if err != nil { printErr(err) return err } - errCh := make(chan error, 1) - go func() { - err := exctr.Execute(req, re, env) - if err != nil { - errCh <- err - } - }() - - select { - case err := <-errCh: + // Execute the command. + err = exctr.Execute(req, re, env) + // If we get an error here, don't bother reading the status from the + // response emitter. It may not even be closed. + if err != nil { printErr(err) if kiterr, ok := err.(*cmdkit.Error); ok { @@ -160,12 +149,10 @@ func Run(ctx context.Context, root *cmds.Command, } return err - - case code := <-exitCh: - if code != 0 { - return ExitError(code) - } } + if code := re.Status(); code != 0 { + return ExitError(code) + } return nil } diff --git a/cli/run_test.go b/cli/run_test.go new file mode 100644 index 00000000..fc502f95 --- /dev/null +++ b/cli/run_test.go @@ -0,0 +1,64 @@ +package cli + +import ( + "context" + "os" + "testing" + "time" + + "github.com/ipfs/go-ipfs-cmds" +) + +var root = &cmds.Command{ + Subcommands: map[string]*cmds.Command{ + "test": &cmds.Command{ + Run: func(req *cmds.Request, re cmds.ResponseEmitter, e cmds.Environment) error { + err := cmds.EmitOnce(re, 42) + + time.Sleep(2 * time.Second) + + e.(env).ch <- struct{}{} + return err + }, + }, + }, +} + +type env struct { + ch chan struct{} +} + +func (e env) Context() context.Context { + return context.Background() +} + +func TestRunWaits(t *testing.T) { + flag := make(chan struct{}, 1) + + devnull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0600) + if err != nil { + t.Fatal(err) + } + defer devnull.Close() + + err = Run( + context.Background(), + root, + []string{"test", "test"}, + devnull, devnull, devnull, + func(ctx context.Context, req *cmds.Request) (cmds.Environment, error) { + return env{flag}, nil + }, + func(req *cmds.Request, env interface{}) (cmds.Executor, error) { + return cmds.NewExecutor(req.Root), nil + }, + ) + if err != nil { + t.Fatal(err) + } + select { + case <-flag: + default: + t.Fatal("expected flag to be raised") + } +} diff --git a/cli/single_test.go b/cli/single_test.go index 33951c76..aff69227 100644 --- a/cli/single_test.go +++ b/cli/single_test.go @@ -16,7 +16,7 @@ func TestSingle(t *testing.T) { var bufout, buferr bytes.Buffer - re, exitCh, err := NewResponseEmitter(&bufout, &buferr, req) + re, err := NewResponseEmitter(&bufout, &buferr, req) if err != nil { t.Fatal(err) } @@ -40,7 +40,9 @@ func TestSingle(t *testing.T) { wait <- struct{}{} }() - exitCode := <-exitCh + <-wait + + exitCode := re.Status() if exitCode != 0 { t.Errorf("expected exit code 0, got: %v", exitCode) } @@ -50,5 +52,4 @@ func TestSingle(t *testing.T) { t.Fatalf("expected %#v, got %#v", "test\n", str) } - <-wait } diff --git a/examples/adder/cmd.go b/examples/adder/cmd.go index f6658723..3b021afb 100644 --- a/examples/adder/cmd.go +++ b/examples/adder/cmd.go @@ -184,8 +184,6 @@ var RootCmd = &cmds.Command{ PostRun: cmds.PostRunMap{ cmds.CLI: func(res cmds.Response, re cmds.ResponseEmitter) error { clire := re.(cli.ResponseEmitter) - - defer re.Close() defer fmt.Println() // length of line at last iteration @@ -193,7 +191,7 @@ var RootCmd = &cmds.Command{ var exit int defer func() { - clire.Exit(exit) + clire.SetStatus(exit) }() for { diff --git a/examples/adder/local/main.go b/examples/adder/local/main.go index ee43a68b..8e7eeafe 100644 --- a/examples/adder/local/main.go +++ b/examples/adder/local/main.go @@ -21,11 +21,13 @@ func main() { req.Options["encoding"] = cmds.Text // create an emitter - re, retCh, err := cli.NewResponseEmitter(os.Stdout, os.Stderr, req) + cliRe, err := cli.NewResponseEmitter(os.Stdout, os.Stderr, req) if err != nil { panic(err) } + wait := make(chan struct{}) + var re cmds.ResponseEmitter = cliRe if pr, ok := req.Command.PostRun[cmds.CLI]; ok { var ( res cmds.Response @@ -35,24 +37,18 @@ func main() { re, res = cmds.NewChanResponsePair(req) go func() { + defer close(wait) err := pr(res, lower) if err != nil { fmt.Println("error: ", err) } }() + } else { + close(wait) } - wait := make(chan struct{}) - // call command in background - go func() { - defer close(wait) - - adder.RootCmd.Call(req, re, nil) - }() - - // wait until command has returned and exit - ret := <-retCh + adder.RootCmd.Call(req, re, nil) <-wait - os.Exit(ret) + os.Exit(cliRe.Status()) } diff --git a/examples/adder/remote/client/main.go b/examples/adder/remote/client/main.go index c481012b..658914f2 100644 --- a/examples/adder/remote/client/main.go +++ b/examples/adder/remote/client/main.go @@ -31,29 +31,20 @@ func main() { req.Options["encoding"] = cmds.Text // create an emitter - re, retCh, err := cli.NewResponseEmitter(os.Stdout, os.Stderr, req) + re, err := cli.NewResponseEmitter(os.Stdout, os.Stderr, req) if err != nil { panic(err) } - wait := make(chan struct{}) // copy received result into cli emitter - go func() { - var err error - - if pr, ok := req.Command.PostRun[cmds.CLI]; ok { - err = pr(res, re) - } else { - err = cmds.Copy(re, res) - } - if err != nil { - re.CloseWithError(err) - } - close(wait) - }() - - // wait until command has returned and exit - ret := <-retCh - <-wait - os.Exit(ret) + if pr, ok := req.Command.PostRun[cmds.CLI]; ok { + err = pr(res, re) + } else { + err = cmds.Copy(re, res) + } + if err != nil { + re.CloseWithError(err) + } + + os.Exit(re.Status()) } From 060c03aafed50783cdc75b43e46701b1dd93c697 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 19 Feb 2019 01:56:25 -0800 Subject: [PATCH 3/4] cli: return the correct parse error --- cli/run.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/run.go b/cli/run.go index f176de59..79f1e900 100644 --- a/cli/run.go +++ b/cli/run.go @@ -92,7 +92,7 @@ func Run(ctx context.Context, root *cmds.Command, printHelp(false, stderr) } - return err + return errParse } // here we handle the cases where From affc2061851259a628e525a167d44f4cb96a5bc5 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 19 Feb 2019 02:18:55 -0800 Subject: [PATCH 4/4] debug: remove error assertions We inserted these while transitioning to the new API but we've never hit one. --- chan.go | 7 ------- cli/responseemitter.go | 7 ------- debug/checkerr.go | 28 ---------------------------- http/responseemitter.go | 7 ------- writer.go | 9 --------- 5 files changed, 58 deletions(-) delete mode 100644 debug/checkerr.go diff --git a/chan.go b/chan.go index be0e8426..7bf83e7d 100644 --- a/chan.go +++ b/chan.go @@ -6,7 +6,6 @@ import ( "sync" "github.com/ipfs/go-ipfs-cmdkit" - "github.com/ipfs/go-ipfs-cmds/debug" ) func NewChanResponsePair(req *Request) (ResponseEmitter, Response) { @@ -130,12 +129,6 @@ func (re *chanResponseEmitter) Emit(v interface{}) error { re.wl.Lock() defer re.wl.Unlock() - // Initially this library allowed commands to return errors by sending an - // error value along a stream. We removed that in favour of CloseWithError, - // so we want to make sure we catch situations where some code still uses the - // old error emitting semantics and _panic_ in those situations. - debug.AssertNotError(v) - // unblock Length() select { case <-re.waitLen: diff --git a/cli/responseemitter.go b/cli/responseemitter.go index 3849be7b..6b882f04 100644 --- a/cli/responseemitter.go +++ b/cli/responseemitter.go @@ -9,7 +9,6 @@ import ( "syscall" "github.com/ipfs/go-ipfs-cmds" - "github.com/ipfs/go-ipfs-cmds/debug" ) var _ ResponseEmitter = &responseEmitter{} @@ -137,12 +136,6 @@ func (re *responseEmitter) Emit(v interface{}) error { isSingle = true } - // Initially this library allowed commands to return errors by sending an - // error value along a stream. We removed that in favour of CloseWithError, - // so we want to make sure we catch situations where some code still uses the - // old error emitting semantics and _panic_ in those situations. - debug.AssertNotError(v) - // channel emission iteration if ch, ok := v.(chan interface{}); ok { v = (<-chan interface{})(ch) diff --git a/debug/checkerr.go b/debug/checkerr.go deleted file mode 100644 index 0e35c932..00000000 --- a/debug/checkerr.go +++ /dev/null @@ -1,28 +0,0 @@ -package debug - -import ( - "fmt" - "runtime/debug" - - "github.com/ipfs/go-ipfs-cmdkit" -) - -// UnexpectedError wraps a *cmdkit.Error that is being emitted. That type (and its non-pointer version) used to be emitted to indicate an error. That was deprecated in favour of CloseWithError. -type UnexpectedError struct { - Err *cmdkit.Error -} - -// Error returns the error string -func (err UnexpectedError) Error() string { - return fmt.Sprintf("unexpected error value emitted: %q at\n%s", err.Err.Error(), debug.Stack()) -} - -// AssertNotError verifies that v is not a cmdkit.Error or *cmdkit.Error. Otherwise it panics. -func AssertNotError(v interface{}) { - if e, ok := v.(cmdkit.Error); ok { - v = &e - } - if e, ok := v.(*cmdkit.Error); ok { - panic(UnexpectedError{e}) - } -} diff --git a/http/responseemitter.go b/http/responseemitter.go index c5e478a3..163da4ae 100644 --- a/http/responseemitter.go +++ b/http/responseemitter.go @@ -10,7 +10,6 @@ import ( "github.com/ipfs/go-ipfs-cmdkit" cmds "github.com/ipfs/go-ipfs-cmds" - "github.com/ipfs/go-ipfs-cmds/debug" ) var ( @@ -88,12 +87,6 @@ type responseEmitter struct { } func (re *responseEmitter) Emit(value interface{}) error { - // Initially this library allowed commands to return errors by sending an - // error value along a stream. We removed that in favour of CloseWithError, - // so we want to make sure we catch situations where some code still uses the - // old error emitting semantics and _panic_ in those situations. - debug.AssertNotError(value) - // if we got a channel, instead emit values received on there. if ch, ok := value.(chan interface{}); ok { value = (<-chan interface{})(ch) diff --git a/writer.go b/writer.go index 6f5fcaad..4b6296f1 100644 --- a/writer.go +++ b/writer.go @@ -8,7 +8,6 @@ import ( "sync" "github.com/ipfs/go-ipfs-cmdkit" - "github.com/ipfs/go-ipfs-cmds/debug" ) func NewWriterResponseEmitter(w io.WriteCloser, req *Request) (ResponseEmitter, error) { @@ -154,14 +153,6 @@ func (re *writerResponseEmitter) Emit(v interface{}) error { return EmitChan(re, ch) } - // Initially this library allowed commands to return errors by sending an - // error value along a stream. We removed that in favour of CloseWithError, - // so we want to make sure we catch situations where some code still uses the - // old error emitting semantics. - // Also errors may occur both as pointers and as plain values, so we need to - // check both. - debug.AssertNotError(v) - if re.closed { return ErrClosedEmitter }