Don't handle timeouts in Execute.

Instead, handle it at the boundaries (in ServeHTTP and the CLI's Run function).
This ensures that we actually *wait* to finish writing everything before
canceling the context.

cli/parse.go

@@ -22,6 +22,8 @@ var log = logging.Logger("cmds/cli")
 
 // Parse parses the input commandline string (cmd, flags, and args).
 // returns the corresponding command Request object.
+//
+// This function never returns nil, even on error.
 func Parse(ctx context.Context, input []string, stdin *os.File, root *cmds.Command) (*cmds.Request, error) {
 	req := &cmds.Request{Context: ctx}
 

cli/run.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"os"
 	"strings"
+	"time"
 
 	"github.com/ipfs/go-ipfs-cmdkit"
 	cmds "github.com/ipfs/go-ipfs-cmds"
@@ -25,6 +26,19 @@ func Run(ctx context.Context, root *cmds.Command,
 
 	req, errParse := Parse(ctx, cmdline[1:], stdin, root)
 
+	// Handle the timeout up front.
+	var cancel func()
+	if timeoutStr, ok := req.Options[cmds.TimeoutOpt]; ok {
+		timeout, err := time.ParseDuration(timeoutStr.(string))
+		if err != nil {
+			return err
+		}
+		req.Context, cancel = context.WithTimeout(req.Context, timeout)
+	} else {
+		req.Context, cancel = context.WithCancel(req.Context)
+	}
+	defer cancel()
+
 	// this is a message to tell the user how to get the help text
 	printMetaHelp := func(w io.Writer) {
 		cmdPath := strings.Join(req.Path, " ")
@@ -79,7 +93,7 @@ func Run(ctx context.Context, root *cmds.Command,
 
 	cmd := req.Command
 
-	env, err := buildEnv(ctx, req)
+	env, err := buildEnv(req.Context, req)
 	if err != nil {
 		printErr(err)
 		return err

executor.go

@@ -2,7 +2,6 @@ package cmds
 
 import (
 	"context"
-	"time"
 
 	"github.com/ipfs/go-ipfs-cmdkit"
 )
@@ -71,16 +70,6 @@ func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) (e
 		}
 	}
 
-	if timeoutStr, ok := req.Options[TimeoutOpt]; ok {
-		timeout, err := time.ParseDuration(timeoutStr.(string))
-		if err != nil {
-			return err
-		}
-		var cancel func()
-		req.Context, cancel = context.WithTimeout(req.Context, timeout)
-		defer cancel()
-	}
-
 	if cmd.PreRun != nil {
 		err = cmd.PreRun(req, env)
 		if err != nil {

http/client.go

@@ -9,7 +9,6 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
-	"time"
 
 	"github.com/ipfs/go-ipfs-cmdkit"
 	"github.com/ipfs/go-ipfs-cmdkit/files"
@@ -161,35 +160,20 @@ func (c *client) Send(req *cmds.Request) (cmds.Response, error) {
 	}
 	httpReq.Header.Set(uaHeader, c.ua)
 
-	var reqCancel func()
-	if timeoutStr, ok := req.Options[cmds.TimeoutOpt]; ok {
-		timeout, err := time.ParseDuration(timeoutStr.(string))
-		if err != nil {
-			return nil, err
-		}
-		req.Context, reqCancel = context.WithTimeout(req.Context, timeout)
-	} else {
-		req.Context, reqCancel = context.WithCancel(req.Context)
-	}
-
 	httpReq = httpReq.WithContext(req.Context)
 	httpReq.Close = true
 
 	httpRes, err := c.httpClient.Do(httpReq)
 	if err != nil {
-		reqCancel()
 		return nil, err
 	}
 
 	// using the overridden JSON encoding in request
 	res, err := parseResponse(httpRes, req)
 	if err != nil {
-		reqCancel()
 		return nil, err
 	}
 
-	res.(*Response).reqCancel = reqCancel
-
 	if found && len(previousUserProvidedEncoding) > 0 {
 		// reset to user provided encoding after sending request
 		// NB: if user has provided an encoding but it is the empty string,

http/handler.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"runtime/debug"
 	"strings"
+	"time"
 
 	"github.com/ipfs/go-ipfs-cmdkit"
 	cmds "github.com/ipfs/go-ipfs-cmds"
@@ -101,21 +102,6 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		ctx = context.Background()
 	}
 
-	ctx, cancel := context.WithCancel(ctx)
-	defer cancel()
-
-	ctx = logging.ContextWithLoggable(ctx, loggables.Uuid("requestId"))
-	if cn, ok := w.(http.CloseNotifier); ok {
-		clientGone := cn.CloseNotify()
-		go func() {
-			select {
-			case <-clientGone:
-			case <-ctx.Done():
-			}
-			cancel()
-		}()
-	}
-
 	if !allowOrigin(r, h.cfg) || !allowReferer(r, h.cfg) {
 		w.WriteHeader(http.StatusForbidden)
 		w.Write([]byte("403 - Forbidden"))
@@ -134,6 +120,31 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Handle the timeout up front.
+	var cancel func()
+	if timeoutStr, ok := req.Options[cmds.TimeoutOpt]; ok {
+		timeout, err := time.ParseDuration(timeoutStr.(string))
+		if err != nil {
+			return
+		}
+		req.Context, cancel = context.WithTimeout(req.Context, timeout)
+	} else {
+		req.Context, cancel = context.WithCancel(req.Context)
+	}
+	defer cancel()
+
+	req.Context = logging.ContextWithLoggable(req.Context, loggables.Uuid("requestId"))
+	if cn, ok := w.(http.CloseNotifier); ok {
+		clientGone := cn.CloseNotify()
+		go func() {
+			select {
+			case <-clientGone:
+			case <-req.Context.Done():
+			}
+			cancel()
+		}()
+	}
+
 	if reqLogger, ok := h.env.(requestLogger); ok {
 		done := reqLogger.LogRequest(req)
 		defer done()

http/response.go

@@ -29,9 +29,6 @@ type Response struct {
 	dec cmds.Decoder
 
 	initErr *cmdkit.Error
-
-	// request context cancel function. might be nil, but we need to call it when we're done
-	reqCancel func()
 }
 
 func (res *Response) Request() *cmds.Request {
@@ -60,9 +57,6 @@ func (res *Response) RawNext() (interface{}, error) {
 	// but only do that once
 	if res.dec == nil {
 		if res.rr == nil {
-			if res.reqCancel != nil {
-				res.reqCancel()
-			}
 			return nil, io.EOF
 		} else {
 			rr := res.rr
@@ -77,9 +71,6 @@ func (res *Response) RawNext() (interface{}, error) {
 	// last error was sent as value, now we get the same error from the headers. ignore and EOF!
 	if err != nil && res.err != nil && err.Error() == res.err.Error() {
 		err = io.EOF
-		if res.reqCancel != nil {
-			res.reqCancel()
-		}
 	}
 
 	return m.Get(), err