Merge pull request #67 from ipfs/feat/cleanup

cleaner command completion/calling

command.go

@@ -66,27 +66,26 @@ var ErrNoFormatter = ClientError("This command cannot be formatted to plain text
 var ErrIncorrectType = errors.New("The command returned a value with a different type than expected")
 
 // Call invokes the command for the given Request
-func (c *Command) Call(req *Request, re ResponseEmitter, env Environment) (err error) {
+func (c *Command) Call(req *Request, re ResponseEmitter, env Environment) {
 	// we need the named return parameter so we can change the value from defer()
-	defer func() {
-		if err == nil {
-			re.Close()
-		}
-	}()
+	defer re.Close()
 
 	var cmd *Command
-	cmd, err = c.Get(req.Path)
+	cmd, err := c.Get(req.Path)
 	if err != nil {
-		return err
+		re.SetError(err, cmdkit.ErrFatal)
+		return
 	}
 
 	if cmd.Run == nil {
-		return ErrNotCallable
+		re.SetError(ErrNotCallable, cmdkit.ErrFatal)
+		return
 	}
 
 	err = cmd.CheckArguments(req)
 	if err != nil {
-		return err
+		re.SetError(err, cmdkit.ErrFatal)
+		return
 	}
 
 	// If this ResponseEmitter encodes messages (e.g. http, cli or writer - but not chan),
@@ -104,23 +103,7 @@ func (c *Command) Call(req *Request, re ResponseEmitter, env Environment) (err e
 		}
 	}
 
-	defer func() {
-		/*
-			// catch panics (esp. from re.SetError)
-			if v := recover(); v != nil {
-				// if they are errors
-				if e, ok := v.(error); ok {
-					// use them as return error
-					err = e
-				}
-				// otherwise keep panicking.
-				panic(v)
-			}
-		*/
-	}()
 	cmd.Run(req, re, env)
-
-	return err
 }
 
 // Resolve returns the subcommands at the given path

command_test.go

 package cmds
 
 import (
-	"bytes"
 	"context"
 	"io"
 	"testing"
@@ -15,12 +14,23 @@ type nopCloser struct{}
 
 func (c nopCloser) Close() error { return nil }
 
-// newBufferResponseEmitter returns a ResponseEmitter that writes
-// into a bytes.Buffer
-func newBufferResponseEmitter() ResponseEmitter {
-	buf := bytes.NewBuffer(nil)
-	wc := writecloser{Writer: buf, Closer: nopCloser{}}
-	return NewWriterResponseEmitter(wc, nil, Encoders[Text])
+type testEmitter testing.T
+
+func (s *testEmitter) Close() error {
+	return nil
+}
+
+func (s *testEmitter) SetLength(_ uint64) {}
+func (s *testEmitter) SetError(err interface{}, code cmdkit.ErrorType) {
+	(*testing.T)(s).Error(err)
+}
+func (s *testEmitter) Emit(value interface{}) error {
+	return nil
+}
+
+// newTestEmitter fails the test if it receives an error.
+func newTestEmitter(t *testing.T) *testEmitter {
+	return (*testEmitter)(t)
 }
 
 // noop does nothing and can be used as a noop Run function
@@ -43,7 +53,7 @@ func TestOptionValidation(t *testing.T) {
 		Run: noop,
 	}
 
-	re := newBufferResponseEmitter()
+	re := newTestEmitter(t)
 	req, err := NewRequest(context.Background(), nil, map[string]interface{}{
 		"beep": true,
 	}, nil, nil, cmd)
@@ -51,19 +61,16 @@ func TestOptionValidation(t *testing.T) {
 		t.Error("Should have failed (incorrect type)")
 	}
 
-	re = newBufferResponseEmitter()
+	re = newTestEmitter(t)
 	req, err = NewRequest(context.Background(), nil, map[string]interface{}{
 		"beep": 5,
 	}, nil, nil, cmd)
 	if err != nil {
 		t.Error(err, "Should have passed")
 	}
-	err = cmd.Call(req, re, nil)
-	if err != nil {
-		t.Error(err, "Should have passed")
-	}
+	cmd.Call(req, re, nil)
 
-	re = newBufferResponseEmitter()
+	re = newTestEmitter(t)
 	req, err = NewRequest(context.Background(), nil, map[string]interface{}{
 		"beep": 5,
 		"boop": "test",
@@ -72,12 +79,9 @@ func TestOptionValidation(t *testing.T) {
 		t.Error("Should have passed")
 	}
 
-	err = cmd.Call(req, re, nil)
-	if err != nil {
-		t.Error("Should have passed")
-	}
+	cmd.Call(req, re, nil)
 
-	re = newBufferResponseEmitter()
+	re = newTestEmitter(t)
 	req, err = NewRequest(context.Background(), nil, map[string]interface{}{
 		"b": 5,
 		"B": "test",
@@ -86,12 +90,9 @@ func TestOptionValidation(t *testing.T) {
 		t.Error("Should have passed")
 	}
 
-	err = cmd.Call(req, re, nil)
-	if err != nil {
-		t.Error("Should have passed")
-	}
+	cmd.Call(req, re, nil)
 
-	re = newBufferResponseEmitter()
+	re = newTestEmitter(t)
 	req, err = NewRequest(context.Background(), nil, map[string]interface{}{
 		"foo": 5,
 	}, nil, nil, cmd)
@@ -99,12 +100,9 @@ func TestOptionValidation(t *testing.T) {
 		t.Error("Should have passed")
 	}
 
-	err = cmd.Call(req, re, nil)
-	if err != nil {
-		t.Error("Should have passed")
-	}
+	cmd.Call(req, re, nil)
 
-	re = newBufferResponseEmitter()
+	re = newTestEmitter(t)
 	req, err = NewRequest(context.Background(), nil, map[string]interface{}{
 		EncLong: "json",
 	}, nil, nil, cmd)
@@ -112,12 +110,9 @@ func TestOptionValidation(t *testing.T) {
 		t.Error("Should have passed")
 	}
 
-	err = cmd.Call(req, re, nil)
-	if err != nil {
-		t.Error("Should have passed")
-	}
+	cmd.Call(req, re, nil)
 
-	re = newBufferResponseEmitter()
+	re = newTestEmitter(t)
 	req, err = NewRequest(context.Background(), nil, map[string]interface{}{
 		"b": "100",
 	}, nil, nil, cmd)
@@ -125,12 +120,9 @@ func TestOptionValidation(t *testing.T) {
 		t.Error("Should have passed")
 	}
 
-	err = cmd.Call(req, re, nil)
-	if err != nil {
-		t.Error("Should have passed")
-	}
+	cmd.Call(req, re, nil)
 
-	re = newBufferResponseEmitter()
+	re = newTestEmitter(t)
 	req, err = NewRequest(context.Background(), nil, map[string]interface{}{
 		"b": ":)",
 	}, nil, nil, cmd)
@@ -337,10 +329,7 @@ func TestPostRun(t *testing.T) {
 		re, res := NewChanResponsePair(req)
 		re = cmd.PostRun[PostRunType(encType)](req, re)
 
-		err = cmd.Call(req, re, nil)
-		if err != nil {
-			t.Fatal(err)
-		}
+		cmd.Call(req, re, nil)
 
 		l := res.Length()
 		if l != tc.finalLength {

examples/adder/local/main.go

@@ -31,10 +31,7 @@ func main() {
 	go func() {
 		defer close(wait)
 
-		err = adder.RootCmd.Call(req, re, nil)
-		if err != nil {
-			panic(err)
-		}
+		adder.RootCmd.Call(req, re, nil)
 	}()
 
 	// wait until command has returned and exit

http/handler.go

@@ -8,7 +8,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/ipfs/go-ipfs-cmdkit"
 	cmds "github.com/ipfs/go-ipfs-cmds"
 	logging "github.com/ipfs/go-log"
 	"github.com/libp2p/go-libp2p-loggables"
@@ -158,24 +157,7 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	re := NewResponseEmitter(w, r.Method, req)
-	defer re.Close()
-
-	// call the command
-	err = h.root.Call(req, re, h.env)
-	if err != nil {
-		re.SetError(err, cmdkit.ErrNormal)
-		return
-	}
-
-	d := re.(Doner)
-	select {
-	case <-d.Done():
-	case <-req.Context.Done():
-		log.Errorf("waiting for http.Responseemitter to close but then %s", req.Context.Err())
-		// too late to send an error, just return
-	}
-
-	return
+	h.root.Call(req, re, h.env)
 }
 
 func sanitizedErrStr(err error) string {

http/responseemitter.go

@@ -26,10 +26,6 @@ var (
 	}
 )
 
-type Doner interface {
-	Done() <-chan struct{}
-}
-
 // NewResponeEmitter returns a new ResponseEmitter.
 func NewResponseEmitter(w http.ResponseWriter, method string, req *cmds.Request) ResponseEmitter {
 	encType := cmds.GetEncoding(req)
@@ -41,12 +37,11 @@ func NewResponseEmitter(w http.ResponseWriter, method string, req *cmds.Request)
 	}
 
 	re := &responseEmitter{
-		w:         w,
-		encType:   encType,
-		enc:       enc,
-		method:    method,
-		req:       req,
-		closeWait: make(chan struct{}),
+		w:       w,
+		encType: encType,
+		enc:     enc,
+		method:  method,
+		req:     req,
 	}
 	return re
 }
@@ -69,9 +64,6 @@ type responseEmitter struct {
 	streaming bool
 	once      sync.Once
 	method    string
-
-	closeOnce sync.Once
-	closeWait chan struct{}
 }
 
 func (re *responseEmitter) Emit(value interface{}) error {
@@ -143,10 +135,6 @@ func (re *responseEmitter) Emit(value interface{}) error {
 	return err
 }
 
-func (re *responseEmitter) Done() <-chan struct{} {
-	return re.closeWait
-}
-
 func (re *responseEmitter) SetLength(l uint64) {
 	h := re.w.Header()
 	h.Set("X-Content-Length", strconv.FormatUint(l, 10))
@@ -156,8 +144,6 @@ func (re *responseEmitter) SetLength(l uint64) {
 
 func (re *responseEmitter) Close() error {
 	re.once.Do(func() { re.preamble(nil) })
-	re.closeOnce.Do(func() { close(re.closeWait) })
-
 	return nil
 }
 
@@ -173,14 +159,9 @@ func (re *responseEmitter) SetError(v interface{}, errType cmdkit.ErrorType) {
 func (re *responseEmitter) Flush() {
 	re.once.Do(func() { re.preamble(nil) })
 
-	select {
-	case <-re.closeWait:
-		log.Error("flush after close")
-		return
-	default:
+	if flusher, ok := re.w.(http.Flusher); ok {
+		flusher.Flush()
 	}
-
-	re.w.(http.Flusher).Flush()
 }
 
 func (re *responseEmitter) preamble(value interface{}) {