Skip to content

Commit

Permalink
core/resolve: addressed code review
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
cryptix committed May 4, 2015
1 parent 93241ed commit 274da9e
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 16 deletions.
27 changes: 22 additions & 5 deletions commands/http/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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

This comment has been minimized.

Copy link
@jbenet

jbenet May 4, 2015

Member

I assume it really isn't because ServeHTTP() doesn't take a pointer receiver, but it's really subtule..

You're right. this tripped me up just now. wow. sketchy. it is correct, but confusing and error prone.

Shouldn't the context be just put on the command request?

hmm maybe? i dont have all the commands lib paged in.

req.SetContext(i.ctx)

// call the command
Expand Down
12 changes: 10 additions & 2 deletions core/pathresolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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

This comment has been minimized.

Copy link
@jbenet

jbenet May 4, 2015

Member

agreed. records should linger locally.

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()
Expand Down
12 changes: 3 additions & 9 deletions path/resolver.go
Original file line number Diff line number Diff line change
@@ -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 (
Expand Down Expand Up @@ -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 {

Expand All @@ -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
Expand Down

0 comments on commit 274da9e

Please sign in to comment.