core/resolve: addressed code review

cryptix · cryptix · commit 6a9be5542121 · 2015-05-04T02:13:10.000+02:00
- pass the nodes context instead of TODO when running commands through the http interface
- error out instead of panic on /ipns/.. resolve in offline mode
- setup ResolveLink timeout for each link
- some minor tweaks to make golint happy
diff --git a/commands/http/handler.go b/commands/http/handler.go
@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
+
 	cmds "github.com/ipfs/go-ipfs/commands"
 	u "github.com/ipfs/go-ipfs/util"
 )
@@ -48,11 +49,6 @@ func NewHandler(ctx cmds.Context, root *cmds.Command, origin string) *Handler {
 }
 
 func (i Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	// create a context.Context to pass into the commands.
-	ctx, cancel := context.WithCancel(context.TODO())
-	defer cancel()
-	i.ctx.Context = ctx
-
 	log.Debug("Incoming API request: ", r.URL)
 
 	// error on external referers (to prevent CSRF attacks)
@@ -84,6 +80,27 @@ func (i Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		w.Write([]byte(err.Error()))
 		return
 	}
+
+	// get the node's context to pass into the commands.
+	node, err := i.ctx.GetNode()
+	if err != nil {
+		err = fmt.Errorf("cmds/http: couldn't GetNode(): %s", err)
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+	ctx, cancel := context.WithCancel(node.Context())
+	defer cancel()
+	/*
+		TODO(cryptix): the next line looks very fishy to me..
+		It looks like the the context for the command request beeing prepared here is shared across all incoming requests..
+
+		I assume it really isn't because ServeHTTP() doesn't take a pointer receiver, but it's really subtule..
+
+		Shouldn't the context be just put on the command request?
+
+		ps: take note of the name clash - commands.Context != context.Context
+	*/
+	i.ctx.Context = ctx
 	req.SetContext(i.ctx)
 
 	// call the command
diff --git a/core/pathresolver.go b/core/pathresolver.go
@@ -13,9 +13,13 @@ import (
 
 const maxLinks = 32
 
-var ErrTooManyLinks = errors.New("exceeded maximum number of links in ipns entry")
+// errors returned by Resolve function
+var (
+	ErrTooManyLinks = errors.New("core/resolve: exceeded maximum number of links in ipns entry")
+	ErrNoNamesys    = errors.New("core/resolve: no Namesys on IpfsNode - can't resolve ipns entry")
+)
 
-// Resolves the given path by parsing out /ipns/ entries and then going
+// Resolve resolves the given path by parsing out /ipns/ entries and then going
 // through the /ipfs/ entries and returning the final merkledage node.
 // Effectively enables /ipns/ in CLI commands.
 func Resolve(ctx context.Context, n *IpfsNode, p path.Path) (*merkledag.Node, error) {
@@ -39,6 +43,10 @@ func (r *resolver) resolveRecurse(depth int) (*merkledag.Node, error) {
 	// to be an ipfs or an ipns resolution?
 
 	if strings.HasPrefix(r.p.String(), "/ipns/") {
+		// TODO(cryptix): we sould be able to query the local cache for the path
+		if r.n.Namesys == nil {
+			return nil, ErrNoNamesys
+		}
 		// if it's an ipns path, try to resolve it.
 		// if we can't, we can give that error back to the user.
 		seg := r.p.Segments()
diff --git a/path/resolver.go b/path/resolver.go
@@ -1,4 +1,4 @@
-// package path implements utilities for resolving paths within ipfs.
+// Package path implements utilities for resolving paths within ipfs.
 package path
 
 import (
@@ -99,9 +99,6 @@ func (s *Resolver) ResolveLinks(ctx context.Context, ndd *merkledag.Node, names
 	result = append(result, ndd)
 	nd := ndd // dup arg workaround
 
-	ctx, cancel := context.WithTimeout(ctx, time.Minute)
-	defer cancel()
-
 	// for each of the path components
 	for _, name := range names {
 
@@ -123,11 +120,8 @@ func (s *Resolver) ResolveLinks(ctx context.Context, ndd *merkledag.Node, names
 
 		if nlink.Node == nil {
 			// fetch object for link and assign to nd
-			/*
-				TODO(cryptix): we wrapped a new context for each name iteration
-				is this correct?
-				moved it out of the loop for now
-			*/
+			ctx, cancel := context.WithTimeout(ctx, time.Minute)
+			defer cancel()
 			nd, err := s.DAG.Get(ctx, next)
 			if err != nil {
 				return append(result, nd), err
