Refactor/http gateway #1191

cryptix · 2015-05-03T03:07:33Z

This is supposed to fix #1150 and clean up the gateway http handler in general. Based on #1189.

This is currently WIP but I need some feedback first, see my TODOcomments in the code.

wking · 2015-05-03T03:14:37Z

core/commands/object.go

@@ -344,39 +344,21 @@ Data should be in the format specified by the --inputenc flag.
 }

 // 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)
+func objectData(n *dag.Node, err error) (io.Reader, error) {


There's so little left in this function after you remove the Resolve call that I'd just drop this (and objectLinks, etc.) entirely. Unless we really want to keep them around for the debug logging…

i think its probably worth dropping, it provides no tangible value added.

Thought so too, this is mixed up from PR#1189 though. I'll remove it there and rebase this once it is merged.

wking · 2015-05-03T03:36:32Z

On Sat, May 02, 2015 at 08:07:34PM -0700, Henry wrote:

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

So I've been thinking about this a bit, and I can't wrap my head
around Contexts. There are lots of calls in Go's standard libraries
that don't take Context calls. Are they all non-blocking? Where do
you draw the line for “we better accept a Context in case the caller
wants to bail”? Is this just for stuff that will eventually get wired
up to a web server where the caller could drop the TCP request? Maybe
we just want a goroutine per request and we can kill the whole
goroutine? Does Go do single-process event looping like Nginx workers
1 and Python's asyncio 2? Is there no more elegant way to stop
processing for a particular request without passing around Context
parameters?

On the other hand, I barely speak Go, so feel free to ignore my
confusion and just carry on writing idiomatic Go (whatever that
happens to be ;).

whyrusleeping · 2015-05-03T03:44:04Z

core/corehttp/gateway_handler.go

@@ -162,7 +116,7 @@ func (i *gatewayHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		errmsg = errmsg + "bad request for " + r.URL.Path
 	}
 	w.Write([]byte(errmsg))
-	log.Debug(errmsg)
+	log.Error(errmsg) // TODO(cryptix): Why are we ignoring handler errors?


we should never ignore errors. any time an error is ignored anywhere in our code, its a bug.

any time an error is ignored anywhere in our code, its a bug.

very much agreed.

though:

logging is not always an ignored error. sometimes there is nothing to do in a messy io environment (e.g. receiving incoming connections from things that are not running the ipfs protocol. there is nothing to do but log/bookkeep).

this error isn't ignored. an HTTP error header is written, and the error itself is written out to the user for feedback. we could do something else for bookkeeping (like add it to a counter somewhere, throw an event, etc), but correctness is ok here.

whyrusleeping · 2015-05-03T03:51:08Z

@wking contexts should be used when you are going to either perform RPC's, spawn off child goroutines you want to handle, or in general when you want to be able to cancel an operation. Not everything needs to take them, but our high level things should take them, otherwise things may hang. Contexts also allow us to more easily clean up after ourselves; cancelling a context says to all your children "stop whatever you are doing, clean up, and exit ASAP"

wking · 2015-05-03T04:12:19Z

On Sat, May 02, 2015 at 08:51:09PM -0700, Jeromy Johnson wrote:

@wking contexts should be used when you are going to either perform
RPC's, spawn off child goroutines you want to handle, or when you
want to be able to cancel an operation. Not everything needs to take
them, but our high level things should take them, otherwise things
may hang.

Lots of things can block besides RPCs and goroutines. Why doesn't
io.Reader.ReadAll() take a Context? Or sync.Mutex.Lock()?

Contexts also allow us to more easily clean up after ourselves;
cancelling a context says to all your children "stop whatever you
are doing, clean up, and exit ASAP"

Which you can do between processes with signals, or in an event-loop
situation just by releasing the state associated with the aborted
request. Are Contexts really the easiest way to do this in Go?

whyrusleeping · 2015-05-03T05:25:13Z

Ill let google explain it better than I can, lol https://blog.golang.org/context

jbenet · 2015-05-03T20:35:45Z

@wking on the contexts-- the reality with Go everything is there's no hard and fast "right" way to do all these things. contexts are very nice in many cases, and very hairy in others (try mixing it with net.Conn's, io, and so on). As with most things, Go tries to cover 80% of the cases really simply, and does a very good job at this. We'll certainly find corner cases where things get complex (and even annoying), but for the most part contexts are a very elegant solution to a difficult problem. Anyway, yeah-- read up on it. and read up on the various takes on it. Take a look at how the examples Sameer gives work(and perhaps some of the more esoteric cases, like the go grpc implementation).

jbenet · 2015-05-03T20:45:16Z

core/corehttp/gateway_handler.go

-	return node, p, err
-}
-
+// TODO(cryptix):  these four helper funcs shoudl also be available elsewhere, i think?


cryptix · 2015-05-08T01:03:11Z

TODO:

Reenable t0111-writable.sh if core: add context.Context param to core.Resolve() #1189 is merged as is

jbenet · 2015-05-08T05:34:54Z

@cryptix I'll rebase this on master so the CR is easier. (last head: 416f435)

jbenet · 2015-05-08T08:31:34Z

core/corehttp/gateway_handler.go

 	}

-	if r.Method == "HEAD" {
+	if r.Method == "GET" || r.Method == "HEAD" {


why not case "GET", "HEAD": in the switch?

the above only is for Writeable. I didn't want to introduce another switch with just one case.

ohhh missed that-- my mistake :)

jbenet · 2015-05-08T08:41:32Z

@cryptix #1189 was merged, so

re-enable t0111-writable.sh ?

cryptix · 2015-05-09T10:22:25Z

rebased

jbenet · 2015-05-09T10:56:01Z

@cryptix may want to squash 1a7b85b wherever it's supposed to go, else some previous commit wont pass all tests

cryptix · 2015-05-09T11:06:09Z

@jbenet Yea, that one was stupid.. i'll squash related once when I'm done.

jbenet · 2015-05-10T12:56:30Z

#1219 merged the changed without the put/writable tests parts. Rebased on top of new master.

Refactor/http gateway

Refactor/http gateway This commit was moved from ipfs/kubo@08ea56c

jbenet added the status/in-progress In progress label May 3, 2015

wking reviewed May 3, 2015
View reviewed changes

whyrusleeping reviewed May 3, 2015
View reviewed changes

cryptix force-pushed the refactor/httpGateway branch from ce4f890 to 2a74b6a Compare May 3, 2015 11:32

jbenet reviewed May 3, 2015
View reviewed changes

cryptix force-pushed the refactor/httpGateway branch from 2a74b6a to 9b60a42 Compare May 4, 2015 11:39

jbenet force-pushed the refactor/httpGateway branch from 416f435 to 34ac0c2 Compare May 8, 2015 05:39

jbenet reviewed May 8, 2015
View reviewed changes

cryptix force-pushed the refactor/httpGateway branch from 230e4c7 to 1a7b85b Compare May 9, 2015 10:20

jbenet mentioned this pull request May 10, 2015

Refactor/http gateway quickfix (1191) #1219

Merged

jbenet force-pushed the refactor/httpGateway branch from 1a7b85b to f867531 Compare May 10, 2015 12:55

http gw: disable PUT and writable tests - again... :(

4537311

cryptix force-pushed the refactor/httpGateway branch from 0823965 to 4537311 Compare May 10, 2015 13:31

cryptix mentioned this pull request May 11, 2015

Sprint F ipfs/team-mgmt#7

Closed

36 tasks

jbenet added a commit that referenced this pull request May 11, 2015

Merge pull request #1191 from ipfs/refactor/httpGateway

08ea56c

Refactor/http gateway

jbenet merged commit 08ea56c into master May 11, 2015

jbenet removed the status/in-progress In progress label May 11, 2015

jbenet deleted the refactor/httpGateway branch May 11, 2015 18:43

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#1191 from ipfs/refactor/httpGateway

2a396c5

Refactor/http gateway This commit was moved from ipfs/kubo@08ea56c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/http gateway #1191

Refactor/http gateway #1191

cryptix commented May 3, 2015

wking May 3, 2015

whyrusleeping May 3, 2015

cryptix May 3, 2015

wking commented May 3, 2015

whyrusleeping May 3, 2015

jbenet May 3, 2015

whyrusleeping commented May 3, 2015

wking commented May 3, 2015

whyrusleeping commented May 3, 2015

jbenet commented May 3, 2015

jbenet May 3, 2015

cryptix commented May 8, 2015

jbenet commented May 8, 2015

jbenet May 8, 2015

cryptix May 8, 2015

jbenet May 8, 2015

jbenet commented May 8, 2015

cryptix commented May 9, 2015

jbenet commented May 9, 2015

cryptix commented May 9, 2015

jbenet commented May 10, 2015

Refactor/http gateway #1191

Refactor/http gateway #1191

Conversation

cryptix commented May 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented May 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented May 3, 2015

wking commented May 3, 2015

whyrusleeping commented May 3, 2015

jbenet commented May 3, 2015

Choose a reason for hiding this comment

cryptix commented May 8, 2015

jbenet commented May 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented May 8, 2015

cryptix commented May 9, 2015

jbenet commented May 9, 2015

cryptix commented May 9, 2015

jbenet commented May 10, 2015