Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: add context.Context param to core.Resolve() #1189

Merged
merged 1 commit into from
May 8, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is super sketchy. I think its probably best to put the context into the request object instead of here on the handler context

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it is put into the request object, but it's put here: line 104. maybe we can find another place to put it, earlier? (nbd for me -- i want to rewire how all this works already)

ps: take note of the name clash - commands.Context != context.Context
*/
i.ctx.Context = ctx
req.SetContext(i.ctx)

// call the command
Expand Down
2 changes: 1 addition & 1 deletion core/commands/cat.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func cat(ctx context.Context, node *core.IpfsNode, paths []string) ([]io.Reader,
readers := make([]io.Reader, 0, len(paths))
length := uint64(0)
for _, fpath := range paths {
dagnode, err := core.Resolve(node, path.Path(fpath))
dagnode, err := core.Resolve(ctx, node, path.Path(fpath))
if err != nil {
return nil, 0, err
}
Expand Down
11 changes: 6 additions & 5 deletions core/commands/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import (
gopath "path"
"strings"

"github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/cheggaaa/pb"
context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"

cmds "github.com/ipfs/go-ipfs/commands"
core "github.com/ipfs/go-ipfs/core"
path "github.com/ipfs/go-ipfs/path"
tar "github.com/ipfs/go-ipfs/thirdparty/tar"
utar "github.com/ipfs/go-ipfs/unixfs/tar"

"github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/cheggaaa/pb"
)

var ErrInvalidCompressionLevel = errors.New("Compression level must be between 1 and 9")
Expand Down Expand Up @@ -62,7 +63,7 @@ may also specify the level of compression by specifying '-l=<1-9>'.
return
}

reader, err := get(node, req.Arguments()[0], cmplvl)
reader, err := get(req.Context().Context, node, req.Arguments()[0], cmplvl)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down Expand Up @@ -165,9 +166,9 @@ func getCompressOptions(req cmds.Request) (int, error) {
return gzip.NoCompression, nil
}

func get(node *core.IpfsNode, p string, compression int) (io.Reader, error) {
func get(ctx context.Context, node *core.IpfsNode, p string, compression int) (io.Reader, error) {
pathToResolve := path.Path(p)
dagnode, err := core.Resolve(node, pathToResolve)
dagnode, err := core.Resolve(ctx, node, pathToResolve)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion core/commands/ls.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ it contains, with the following format:

dagnodes := make([]*merkledag.Node, 0)
for _, fpath := range paths {
dagnode, err := core.Resolve(node, path.Path(fpath))
dagnode, err := core.Resolve(req.Context().Context, node, path.Path(fpath))
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down
51 changes: 10 additions & 41 deletions core/commands/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,12 @@ output is the raw data of the object.
}

fpath := path.Path(req.Arguments()[0])
output, err := objectData(n, fpath)
node, err := core.Resolve(req.Context().Context, n, fpath)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}
res.SetOutput(output)
res.SetOutput(bytes.NewReader(node.Data))
},
}

Expand All @@ -121,7 +121,12 @@ multihash.
}

fpath := path.Path(req.Arguments()[0])
output, err := objectLinks(n, fpath)
node, err := core.Resolve(req.Context().Context, n, fpath)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
}
output, err := getOutput(node)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down Expand Up @@ -176,7 +181,7 @@ This command outputs data in the following encodings:

fpath := path.Path(req.Arguments()[0])

object, err := objectGet(n, fpath)
object, err := core.Resolve(req.Context().Context, n, fpath)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down Expand Up @@ -242,7 +247,7 @@ var objectStatCmd = &cmds.Command{

fpath := path.Path(req.Arguments()[0])

object, err := objectGet(n, fpath)
object, err := core.Resolve(req.Context().Context, n, fpath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels strange to me... not sure whether or not i like the pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see my comment below (on gh..). I have no strong feelings on this, can change it back as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, removed all of the helpers.

if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down Expand Up @@ -343,42 +348,6 @@ Data should be in the format specified by the --inputenc flag.
Type: Object{},
}

// objectData takes a key string and writes out the raw bytes of that node (if there is one)
func objectData(n *core.IpfsNode, fpath path.Path) (io.Reader, error) {
dagnode, err := core.Resolve(n, fpath)
if err != nil {
return nil, err
}

log.Debugf("objectData: found dagnode %s (# of bytes: %d - # links: %d)", fpath, len(dagnode.Data), len(dagnode.Links))

return bytes.NewReader(dagnode.Data), nil
}

// objectLinks takes a key string and lists the links it points to
func objectLinks(n *core.IpfsNode, fpath path.Path) (*Object, error) {
dagnode, err := core.Resolve(n, fpath)
if err != nil {
return nil, err
}

log.Debugf("objectLinks: found dagnode %s (# of bytes: %d - # links: %d)", fpath, len(dagnode.Data), len(dagnode.Links))

return getOutput(dagnode)
}

// objectGet takes a key string from args and a format option and serializes the dagnode to that format
func objectGet(n *core.IpfsNode, fpath path.Path) (*dag.Node, error) {
dagnode, err := core.Resolve(n, fpath)
if err != nil {
return nil, err
}

log.Debugf("objectGet: found dagnode %s (# of bytes: %d - # links: %d)", fpath, len(dagnode.Data), len(dagnode.Links))

return dagnode, nil
}

// ErrEmptyNode is returned when the input to 'ipfs object put' contains no data
var ErrEmptyNode = errors.New("no data or links in this node")

Expand Down
11 changes: 7 additions & 4 deletions core/commands/publish.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"io"
"strings"

context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"

cmds "github.com/ipfs/go-ipfs/commands"
core "github.com/ipfs/go-ipfs/core"
crypto "github.com/ipfs/go-ipfs/p2p/crypto"
Expand Down Expand Up @@ -89,7 +91,8 @@ Publish an <ipfs-path> to another public key (not implemented):
}

// TODO n.Keychain.Get(name).PrivKey
output, err := publish(n, n.PrivateKey, p)
// TODO(cryptix): is req.Context().Context a child of n.Context()?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the top level context stuff around here is really confusing.. it uses this context group thing if i remember correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbenet can you tell us?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently req.Context().Context() is not yet a child of n.Context(): https://github.com/ipfs/go-ipfs/blob/master/commands/http/handler.go#L52-L54 -- it should be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and yes it's very confusing :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbenet ohai cmdlib 😞 .. but fixed 😄. Do I need to mirror this change on the offline mode, too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cryptix

Do I need to mirror this change on the offline mode, too?

probably? i can think about this more carefully tomorrow.

output, err := publish(req.Context().Context, n, n.PrivateKey, p)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand All @@ -106,14 +109,14 @@ Publish an <ipfs-path> to another public key (not implemented):
Type: IpnsEntry{},
}

func publish(n *core.IpfsNode, k crypto.PrivKey, ref path.Path) (*IpnsEntry, error) {
func publish(ctx context.Context, n *core.IpfsNode, k crypto.PrivKey, ref path.Path) (*IpnsEntry, error) {
// First, verify the path exists
_, err := core.Resolve(n, ref)
_, err := core.Resolve(ctx, n, ref)
if err != nil {
return nil, err
}

err = n.Namesys.Publish(n.Context(), k, ref)
err = n.Namesys.Publish(ctx, k, ref)
if err != nil {
return nil, err
}
Expand Down
6 changes: 3 additions & 3 deletions core/commands/refs.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ Note: list all refs recursively with -r.
return
}

objs, err := objectsForPaths(n, req.Arguments())
objs, err := objectsForPaths(ctx, n, req.Arguments())
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down Expand Up @@ -161,10 +161,10 @@ Displays the hashes of all local objects.
},
}

func objectsForPaths(n *core.IpfsNode, paths []string) ([]*dag.Node, error) {
func objectsForPaths(ctx context.Context, n *core.IpfsNode, paths []string) ([]*dag.Node, error) {
objects := make([]*dag.Node, len(paths))
for i, p := range paths {
o, err := core.Resolve(n, path.Path(p))
o, err := core.Resolve(ctx, n, path.Path(p))
if err != nil {
return nil, err
}
Expand Down
10 changes: 7 additions & 3 deletions core/corehttp/gateway_handler.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package corehttp

import (
"errors"
"fmt"
"html/template"
"io"
Expand Down Expand Up @@ -101,7 +102,7 @@ func (i *gatewayHandler) ResolvePath(ctx context.Context, p string) (*dag.Node,
return nil, "", err
}

node, err := i.node.Resolver.ResolvePath(path.Path(p))
node, err := i.node.Resolver.ResolvePath(ctx, path.Path(p))
if err != nil {
return nil, "", err
}
Expand Down Expand Up @@ -309,6 +310,9 @@ func (i *gatewayHandler) putEmptyDirHandler(w http.ResponseWriter, r *http.Reque
}

func (i *gatewayHandler) putHandler(w http.ResponseWriter, r *http.Request) {
// TODO(cryptix): will be resolved in PR#1191
webErrorWithCode(w, "Sorry, PUT is bugged right now, closing request", errors.New("handler disabled"), http.StatusInternalServerError)
return
urlPath := r.URL.Path
pathext := urlPath[5:]
var err error
Expand Down Expand Up @@ -362,7 +366,7 @@ func (i *gatewayHandler) putHandler(w http.ResponseWriter, r *http.Request) {

// resolving path components into merkledag nodes. if a component does not
// resolve, create empty directories (which will be linked and populated below.)
path_nodes, err := i.node.Resolver.ResolveLinks(rootnd, components[:len(components)-1])
path_nodes, err := i.node.Resolver.ResolveLinks(tctx, rootnd, components[:len(components)-1])
if _, ok := err.(path.ErrNoLink); ok {
// Create empty directories, links will be made further down the code
for len(path_nodes) < len(components) {
Expand Down Expand Up @@ -424,7 +428,7 @@ func (i *gatewayHandler) deleteHandler(w http.ResponseWriter, r *http.Request) {
return
}

path_nodes, err := i.node.Resolver.ResolveLinks(rootnd, components[:len(components)-1])
path_nodes, err := i.node.Resolver.ResolveLinks(tctx, rootnd, components[:len(components)-1])
if err != nil {
webError(w, "Could not resolve parent object", err, http.StatusBadRequest)
return
Expand Down
12 changes: 8 additions & 4 deletions core/corerepo/pinning.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@ import (
)

func Pin(n *core.IpfsNode, paths []string, recursive bool) ([]u.Key, error) {
// TODO(cryptix): do we want a ctx as first param for (Un)Pin() as well, just like core.Resolve?
ctx := n.Context()

dagnodes := make([]*merkledag.Node, 0)
for _, fpath := range paths {
dagnode, err := core.Resolve(n, path.Path(fpath))
dagnode, err := core.Resolve(ctx, n, path.Path(fpath))
if err != nil {
return nil, fmt.Errorf("pin: %s", err)
}
Expand All @@ -43,7 +45,7 @@ func Pin(n *core.IpfsNode, paths []string, recursive bool) ([]u.Key, error) {
return nil, err
}

ctx, cancel := context.WithTimeout(context.TODO(), time.Minute)
ctx, cancel := context.WithTimeout(ctx, time.Minute)
defer cancel()
err = n.Pinning.Pin(ctx, dagnode, recursive)
if err != nil {
Expand All @@ -61,10 +63,12 @@ func Pin(n *core.IpfsNode, paths []string, recursive bool) ([]u.Key, error) {
}

func Unpin(n *core.IpfsNode, paths []string, recursive bool) ([]u.Key, error) {
// TODO(cryptix): do we want a ctx as first param for (Un)Pin() as well, just like core.Resolve?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most likely, yeah. I'd also be inclined to move Unpin towards taking only a single path and moving the iteration logic upwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Next PR though, this is getting big.

ctx := n.Context()

dagnodes := make([]*merkledag.Node, 0)
for _, fpath := range paths {
dagnode, err := core.Resolve(n, path.Path(fpath))
dagnode, err := core.Resolve(ctx, n, path.Path(fpath))
if err != nil {
return nil, err
}
Expand All @@ -75,7 +79,7 @@ func Unpin(n *core.IpfsNode, paths []string, recursive bool) ([]u.Key, error) {
for _, dagnode := range dagnodes {
k, _ := dagnode.Key()

ctx, cancel := context.WithTimeout(context.TODO(), time.Minute)
ctx, cancel := context.WithTimeout(ctx, time.Minute)
defer cancel()
err := n.Pinning.Unpin(ctx, k, recursive)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion core/coreunix/cat.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

func Cat(n *core.IpfsNode, pstr string) (io.Reader, error) {
p := path.FromString(pstr)
dagNode, err := n.Resolver.ResolvePath(p)
dagNode, err := n.Resolver.ResolvePath(n.ContextGroup.Context(), p)
if err != nil {
return nil, err
}
Expand Down
Loading