Bundle of changes #112

keks · 2018-05-24T11:32:01Z

This branch bundles a couple of changes so we can test against all of them at once in go-ipfs

Improve Run function signature to improve error handling #86 Let cmd.Run return an error (PR Let Run return an error to be emitted #97)
Rename SetError to CloseWithError #98 Remove SetError and add CloseWithError
- also removes HandleError, RecvdErr and RawNext
- PRs: [WIP] Change SetError to CloseWithError #103, Remove Response.RawNext #110
Change PostRun signature #102 Change PostRun signature so user don't have to build the chanresponse themselves

keks · 2018-05-25T12:02:28Z

in one of the PRs above you said you wanted the PR bundled and the changes to go-ipfs ready. This is now done! I'm pretty pleased with the result, especially since it removes some odd stuff like that function HandleError or the method RawNext on ResponseEmitter.

The changes to go-ipfs are in ipfs/kubo#5035.

whyrusleeping · 2018-05-28T05:19:03Z

cli/responseemitter.go

@@ -70,26 +74,52 @@ func (re *responseEmitter) SetEncoder(enc func(io.Writer) cmds.Encoder) {
 	re.enc = enc(re.stdout)
 }

-func (re *responseEmitter) SetError(v interface{}, errType cmdkit.ErrorType) {
+func (re *responseEmitter) CloseWithError(err error) error {


It would be really great if this method didnt return an error. I can't think of any situation where the caller wants to do something with the error, or even can. I think it's likely better to just log an error and move on.

The reason I let made this return an error is that CloseWithError(error) error is idiomatic and is used throughout the entire Go ecosystem. I don't see a reason to diverge - maybe we don't really need it in ipfs, but possible somewhere else. And now that the command's Run function just returns an error we don't call this so often that it's too much of a pain for the caller to do the logging themselves.

keks · 2018-05-31T11:54:35Z

Hey @whyrusleeping judging by Stebalien's contribution activity graph it seems they are currently not avalailable, so could you maybe review this PR or delegate this? Thanks!

keks · 2018-05-31T11:54:59Z

Also if you have questions on the structure of this bundle PR, shoot!

whyrusleeping · 2018-05-31T12:01:28Z

@keks yeah, he's just now getting back from a vacation. I'll still want his review, but i'll take a pass here :)

whyrusleeping · 2018-05-31T12:02:45Z

chan.go

-			r.err = val
-			return nil, ErrRcvdError
+			// TODO keks remove logging
+			log.Error("unexpected error value:", val)


whyrusleeping · 2018-05-31T12:03:20Z

chan.go


 func (re *chanResponseEmitter) Emit(v interface{}) error {
+	if e, ok := v.(*cmdkit.Error); ok {


this is because we shouldnt be emitting errors ever anymore, right?

Exactly. Should I remove this or are we not confident enough this doesn't happen? Or should I return that as an error, so it will definitely fail?

i think we should keep it, and maybe even add a check for the non-pointerized version too just in case. Then also add a comment explaining why this bit of code is here.

keks · 2018-06-09T10:49:01Z

Hey @Stebalien, do you think you'll find some time to CR this PR and the one in go-ipfs? Would love to merge this soon

whyrusleeping · 2018-06-11T05:53:32Z

chan.go

@@ -70,6 +82,20 @@ func (r *chanResponse) Length() uint64 {
 	return r.length
 }

+func (re *chanResponse) Head() Head {


Whats this method supposed to do?

It's not new, I just moved it from here: https://github.com/ipfs/go-ipfs-cmds/pull/112/files/15c1508126b5934ffae976e4a09c4353fa90956e#diff-d1319de8df19ba21c67a62fa1fde5f89L201

But I think it's needed for HEAD requests, it's even in the ResponseEmitter interface; but it looks like it's not called from anywhere in go-ipfs-cmds/..., so maybe we can drop it. I'd suggest doing that in a new PR though.

I decided to do it in this PR.

whyrusleeping · 2018-06-13T15:24:26Z

http/response.go

@@ -73,7 +76,24 @@ func (res *Response) RawNext() (interface{}, error) {
 		err = io.EOF
 	}

-	return m.Get(), err
+	if err != nil {


maybe put this inside the same if err != nil as above? might make it a bit clearer as to whats going on here.

Stebalien

First pass.

Stebalien · 2018-06-21T00:05:39Z

chan.go

@@ -57,7 +61,15 @@ func (r *chanResponse) Error() *cmdkit.Error {
 		return nil


Can we get rid of both of these nil tests? I'd rathe panic if something is wrong (and we'll panic in the second case anyways).

well the r == nil check obviously doesn't help because we already access r.wait before. But what's wrong with the r.err == nil check?

We call r.err.Error() below, so r.err better not be nil.

Sorry, I was refering to both of the r == nil checks (in this function and the one above). Unfortunately, git hides that part of the patch.

Stebalien · 2018-06-21T00:09:16Z

chan.go

+func (re *chanResponse) Head() Head {
+	<-re.wait
+
+	err, ok := re.err.(*cmdkit.Error)


This will treat EOF as an error. Maybe we should just have a separate closed flag if we need that and not write EOF to the error field.

Actually, we can just check <-re.closed.

You are right that this is not correct. However, there can be an error even if re.closed already is closed, so I don't think it's a good indicator. I think I fixed this locally and will push the changes once I'm through you first pass.

Stebalien · 2018-06-21T00:11:54Z

chan.go

 	case v, ok := <-r.ch:
 		if !ok {
+			r.ch = nil


Are you sure this isn't racy? Also, this'll cause this method to hang forever if called again.

Stebalien · 2018-06-21T00:13:13Z

chan.go

 			return nil, io.EOF
 		}

-		if err, ok := v.(cmdkit.Error); ok {
+		if err, ok := v.(*cmdkit.Error); ok {


I'm pretty sure we just want to remove this entirely.

I'm not sure, here whyrusleeping argued we should check both all the time. There's an issue on fixing this for cmdkit.Error in general, but there was little feedback.

Also, that line is wrong, should be without the dereference. I'll fix that.

We're already checking if something is an error before putting it into the channel. If we leave this here, we'll need to leve the cmdkit.Error case in the switch statement below. However, IMO, we should just remove both (there shouldn't be any errors here).

Stebalien · 2018-06-21T00:16:12Z

chan.go

+	}
+	if e, ok := v.(*cmdkit.Error); ok {
+		log.Errorf("unexpected error value emitted: %s at\n%s", e.Error(), debug.Stack())
+	}


Shouldn't this call CloseWithError and return?

Nah, we don't emit errors anymore. Doing so is a programmer error, so we want them to know. But panicking because of this feels too strong, so I opted for logging an error.

That won't cause a panic, it'll just "do the right thing" (redirect the error). As it stands, we'll end up sending the error on the value channel.

Oh yes, I meant to return. Anyway, I change it to panic now. I hope we can then in the near future remove those panics because we are more confident we don't do that mistake anymore.

Stebalien · 2018-06-21T00:30:46Z

chan.go

+	})
+
+	re.l.Lock()
+	defer re.l.Unlock()

 	if re.ch == nil {


I don't think this is a valid "closed" check anymore. Probably want to check:

select { case <-re.closed: return ... default: }

You're right it's not a valid close check anymore, but I think your proposal is also racy: It might occur that re.closed is not yet closed but re.ch is, which would result in a write to closed channel error. Thinking this through...

It should be if we only close the channel while holding the lock.

This is actually a good case for removing the closeOnce sync.Once (a user may think that closeOnce is protecting the close while really the lock is).

Stebalien · 2018-06-21T00:37:33Z

cli/responseemitter.go

-
-		return nil
-
+		log.Errorf("got an error: %v\n%s", err, debug.Stack())


Again, we should probably forward these errors to CloseWithError for now.

I believe we should do away with emitting errors and make sure doing so is an error.

We should. However, we should either panic in cases like this or "do the right thing". Simply logging and dropping the error on the floor is not going to end well.

Stebalien · 2018-06-21T00:38:38Z

cli/responseemitter.go

 	case io.Reader:
 		_, err = io.Copy(re.stdout, t)
 		if err != nil {
-			re.SetError(err, cmdkit.ErrNormal)
-			err = nil
+			return err


I'm wondering if this should also call CloseWithError. We could also say that it's the callers responsibility to return the error from Run. Thoughts?

Calling CloseWithError here would close the emitter and only print io.Copy's error. I think it's better to allow the caller to specify a more intelligible error.

Fair enough.

Stebalien · 2018-06-21T00:38:56Z

cli/responseemitter_test.go

 	//"io"
 	"testing"

-	"github.com/ipfs/go-ipfs-cmdkit"
+	//"github.com/ipfs/go-ipfs-cmdkit"


Should probably remove before final merge.

Stebalien · 2018-06-21T00:50:16Z

command.go

-	defer re.Close()
+	var err error
+
+	defer func() {


How about putting all the logic in Call in a separate call function that turns an error. That way, we don't need this defer? The shared err variable makes be uncomfortable.

Stebalien

Second round.

Stebalien · 2018-06-21T01:15:29Z

executor.go

-	return nil
+	err = cmd.Run(req, re, env)
+	log.Debugf("cmds.Execute: Run returned %q with response emitter of type %T", err, re)
+	if err == nil {


Can just call CloseWithError (with a nil error).

executor.go

+
+			re, res = NewChanResponsePair(req)
+
+			go func() {


Stebalien · 2018-06-21T01:20:29Z

http/client.go

+				return re.CloseWithError(err)
+			}
+
+			return re.Close()


Again, can probably just call CloseWithError.

Stebalien · 2018-06-21T01:22:08Z

http/response.go

-	e := res.err
-	res.err = nil
-	return e
+	if res.err == io.EOF {


Impossible condition.

Stebalien · 2018-06-21T06:25:05Z

http/response.go

 	}

-	return m.Get(), err
+	return v, err
 }

 func (res *Response) Next() (interface{}, error) {


THis function checks if the value is an error, I assume this isn't possible given RawNext?

Stebalien · 2018-06-21T06:26:29Z

http/responseemitter.go

+		e = &cmdkit.Error{Message: err.Error()}
+	}
+
+	if re.streaming || e.Code == cmdkit.ErrFatal {


Aren't all errors "fatal" now? That is, can't we just always use the trailer now?

Stebalien · 2018-06-21T06:27:19Z

writer.go

 	case Single:
 		return val.Value, nil
+	case *cmdkit.Error:


Again, I assume this case is impossible.

Stebalien · 2018-06-21T06:28:41Z

Nice! This is starting to look a lot more robust.

keks · 2018-06-29T17:34:07Z

@Stebalien thank you for the thorough feedback! I tried to incorporate all of it and made some additional changes:

remove the Header interface and the Head() methods from all Response types. It wasn't being used anywhere.
remove the RawNext methods from all Response types
remove the HandleError function
unexport WriterResponseEmitter
add a new global error variable ErrClosedEmitter that is meant to be returned by Emit and Close{,WithError}.
work a little bit on testing. More to come in a different PR.

Right now Travis complains that writer.go is not gofmt'd. However, when doing go fmt ./..., no file is changed. I'm not sure what's going on here.

Also, in my branch of go-ipfs that has the changes for this library there is a test (this one) that doesn't complete currently. Investigating that now. Also my branch fell behind a bit, so I'll still need to rebase that on master -- but I first want to get that test to pass.

keks · 2018-06-30T15:41:27Z

Okay, I fixed all the tests in the go-ipfs branch and rebased it is it's en par with master.

I still have no clue with Travis let's this PR fail though. It thinks that writer.go is not gofmt'd, even though it is -- at least for me calling go fmt doesn't change anything. Rebuilding doesn't change anything.
Has anyone of you stumbled over something like this in the past? My go installation is up to date.

keks · 2018-06-30T15:41:38Z

cc @Stebalien

Stebalien · 2018-07-02T20:10:41Z

chan.go

+	defer re.wl.Unlock()
+
+	if _, ok := v.(Single); ok {
+		defer re.closeWithError(nil)


This'll close without error even if we fail somewhere down the line. It can also panic if we emit a single value after closing.

Stebalien · 2018-07-02T20:11:16Z

chan.go


+func (r *chanResponse) Request() *Request {
 	return r.req
 }

 func (r *chanResponse) Error() *cmdkit.Error {
 	<-r.wait


It seems kind of funny for this method to wait for at least one value to be emitted. I'd expect it to either wait until the end or always return immediately (preferably the latter to reduce the chance of a deadlock).

I understand the concern about deadlocks, but returning immediately raises the question of what to return when the command is still running and we didn't see an error yet. So I went for wait-until-close, I'm currently testing this in go-ipfs to see if it makes problems.

Okay, if we block on error until the stream is closed and change nothing else, we get a deadlock. Now I just return nil if the connection is not closed yet.

Stebalien · 2018-07-02T20:11:37Z

chan.go

 	return r.req
 }

 func (r *chanResponse) Error() *cmdkit.Error {
 	<-r.wait

-	if r == nil {


This is racy as we set err under a lock (and can set it at any time).

Stebalien · 2018-07-02T20:19:26Z

cli/responseemitter.go

-		Err: re.err,
-	}
-}
-
 func (re *responseEmitter) Emit(v interface{}) error {
 	// unwrap
 	if val, ok := v.(cmds.Single); ok {
 		v = val.Value


Do we not need to close in this case?

executor.go

+
+				err := cmd.PostRun[typer.Type()](res, lower)
+				closeErr = lower.CloseWithError(err)
+				errCh <- closeErr


Stebalien · 2018-07-02T20:55:34Z

executor.go

 			// if they are errors
-			if e, ok := v.(error); ok {
+			if err, ok := v.(error); ok {


This is throwing the error away, not returning it. It's creating a new error variable.

I know it's creating a new error variable, and I use it as the argument when calling CloseWithError. I don't think I'm throwing it away?

Ah. Got it. I thought you were trying to overwrite the named return value err.

Stebalien · 2018-07-02T20:56:04Z

executor.go

@@ -105,6 +132,12 @@ func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) (e
 		}



So, Run panics, we won't wait for PostRun to exit. Not much of an issue but we may want to consider waiting on errCh in here instead.

I'd still want to return the error from errCh, but yes the defer should also wait for it. So let's wait in both places.

Stebalien · 2018-07-02T22:06:01Z

http/responseemitter.go

-		ch, isChan = value.(chan interface{})
+	if single, ok := value.(cmds.Single); ok {
+		value = single.Value
+		defer re.closeWithError(nil)


Probably needs to go after the lock.

Stebalien · 2018-07-02T22:08:28Z

http/responseemitter.go

-	if _, ok := value.(cmdkit.Error); ok {
-		log.Warning("fixme: got Error not *Error: ", value)
-		value = &value
+	if err, ok := value.(cmdkit.Error); ok {


Use the debug error assertion?

Stebalien · 2018-07-02T22:18:47Z

writer.go

+	return errors.New("provided closer does not support CloseWithError")
+}
+
+func (re *writerResponseEmitter) SetError(v interface{}, errType cmdkit.ErrorType) {
 	err := re.Emit(&cmdkit.Error{Message: fmt.Sprint(v), Code: errType})


Shouldn't we remove this function?

We can't just remove it, because then it wouldn't satisfy the interface anymore. But I made it simply log an error.

Stebalien

We need to consider the story around calling Close and CloseWithError. We could tell users not to do that and to simply return the errors from Run or we could say the first one wins.

If we say the first one wins, we'll need to fix several of the CloseWithError implementations so that they don't barf when called twice. We'll also want to avoid logging to noisily when we accidentally close twice.

If we do say "don't close, well do it for you", then we'd need to figure out how to handle Single as that does close.

Stebalien · 2018-07-19T22:53:24Z

executor.go

 			// if they are errors
-			if e, ok := v.(error); ok {
+			if err, ok := v.(error); ok {


Ah. Got it. I thought you were trying to overwrite the named return value err.

Stebalien · 2018-07-19T22:57:25Z

chan.go

@@ -155,6 +151,10 @@ func (re *chanResponseEmitter) Emit(v interface{}) error {

 	select {
 	case re.ch <- v:
+		if _, ok := v.(Single); ok {


Nit: We could unwrap this before sending to simplify the Next() implementation. However, that's up to you (and may not save any code).

Stebalien · 2018-07-19T23:12:44Z

chan.go

+	// waitErr is closed when the stream is closed.
+	// Error checks if the stream has been closed by checking if this channes is closed.
+	// Its closing is protected by wl.
+	waitErr chan struct{}


Nit: I'd just call this closedCh or something like that. waitErr implies some relation to the error.

Stebalien · 2018-07-19T23:16:35Z

chan.go

 		case Single:
 			return val.Value, nil
 		default:
 			return v, nil
 		}
 	case <-ctx.Done():
-		close(r.done)


So, there's a subtle but probably harmless issue here: Next() can return an error while re.Error() won't. We may want to close here (with ctx.Err()?).

I'm not sure. That would mean that if one call to Next() gets cancelled, the stream will end. Intuitively I would say that the stream should live on and the caller should be able to do another call to Next() later. Don't you think?

Ah, no, you're totally right.

Stebalien · 2018-07-19T23:29:51Z

chan.go


-	if re.ch == nil {
-		return fmt.Errorf("emitter closed")
+	// unblock Length() and Error()


Doesn't unblock Error().

Should still fix this comment.

Stebalien · 2018-07-19T23:36:19Z

cli/responseemitter.go

+		return re.Close()
+	}
+
+	e, ok := err.(*cmdkit.Error)


We should probably be consistent with the cmdkit.Error -> *cmdkit.Error dance. We should either do that everywhere or nowhere.

This still looks like an issue. We should probably have a "if cmdkit.Error, convert to *cmdkit.Error" check here.

Stebalien · 2018-07-19T23:49:01Z

cli/responseemitter.go

@@ -200,6 +211,10 @@ func (re *responseEmitter) Emit(v interface{}) error {
 		}
 	}

+	if isSingle {
+		return re.Close()


This'll return success to the user if, e.g., encoding fails. Should probably call re.CloseWithErr(err).

This still looks like an issue.

Stebalien · 2018-07-20T00:01:07Z

writer.go

+	// old error emitting semantics.
+	// Also errors may occur both as pointers and as plain values, so we need to
+	// check both.
+	debug.AssertNotError(v)

 	if _, ok := v.(Single); ok {
 		defer re.Close()


Will close without error if encoding fails.

I don't think I would send the encoding error down the stream. If I try to send an unencodable value through a writer, the stream should live on and I should get an error so I can send something else.

However, we should return the Close() error, if there is one.

Stebalien · 2018-07-20T00:04:20Z

writer.go

-func (re *WriterResponseEmitter) SetLength(length uint64) {
+func (re *writerResponseEmitter) SetError(v interface{}, errType cmdkit.ErrorType) {
+	err := &cmdkit.Error{Message: fmt.Sprint(v), Code: errType}
+	log.Errorf("error setting error %q: cannot set error with writerResponseEmitter", err)


If we can't do the right thing here, maybe we should just remove this from the interface. We really don't want the user to try calling this and then return a "success" to the user.

Actually, hasn't this been removed from the interface?

Yes, looks like I forgot deleting it. done.

Stebalien · 2018-07-20T00:15:31Z

http/responseemitter.go

+
+	if single, ok := value.(cmds.Single); ok {
+		value = single.Value
+		defer re.closeWithError(nil)


Will throw away encoding errors.

IMHO encoding errors should be passed to the caller instead of be used to close the stream. And closeWithError always returns nil so I figured we don't need it to handle the error.

The issue is that we're closing the response stream as if the command succeeded when, really, the command failed.

Stebalien · 2018-07-20T00:20:56Z

FYI, if I take more than a week to get to a review, the notification has been buried in my inbox. At that point, just bug me on IRC (really, feel free to bug me on IRC repeatedly as soon as you have something for me to review).

keks · 2018-07-27T10:53:29Z

Thank you for the review. I was traveling and will work on these over the next week. And okay, next time I'll be more stubborn :)

keks · 2018-08-08T13:14:55Z

Hey @Stebalien I now commented or acted on everything except the thoughts on Single. I'm still pondering on that one, especially whether it's possible to only set the headers that this is a single value, but still leave the closing to the caller. Otherwise making Close() idempotent, i.e. closing a closed channel doesn't do anything, also seems like a good idea. Giving that some thought right now.

keks · 2018-08-08T13:55:43Z

Stebalien · 2018-08-09T19:38:10Z

ignore this error in

Call
Execute

And possibly some places in go-ipfs.

Warning, @whyrusleeping is grumbling about this being a massive breaking change. My plan is:

Finish up this PR but don't merge it:
Try patching go-ipfs.
If the patch is too hard to write or is unreviewable:
1. Make the Run function an interface{}.
2. Type switch on error/no error.
3. Add back a SetError function that just calls CloseWithError under the hood.

- setting the error is moved into the preamble - we call the preamble before the err!=nil branch because we send to the conn in there

This reverts commit a011832. Do not remove tests that reveal race conditions.

shadow err

Stebalien · 2018-08-27T21:16:15Z

So, it turns out I was wrong about how we currently handle HTTP headers/errors. We always handle trailing errors using the responseReader helper (so we don't need to do that twice). My comment was incorrect: #112 (comment)

Given this, I believe we can switch to sending errors-in-trailers only for all 200 responses (don't additionally include the value). Thoughts? This should cover both the old client talking to a new server and a new server talking to an old client cases.

I'd like to eventually move to:

Sending a non-200 response on parse failure or command not found (returning the error as a plain string in the response).
Sending a 200 response in all other cases, returning any errors in a trailer.

But we can tackle this later.

keks · 2018-08-31T10:50:49Z

@Stebalien done! Want to give it another round of review?

Stebalien · 2018-09-07T01:35:58Z

http/parse.go

@@ -218,7 +218,10 @@ func parseResponse(httpRes *http.Response, req *cmds.Request) (cmds.Response, er
 			return nil, fmt.Errorf("unknown error content type: %s", contentType)
 		default:
 			// handle errors from headers
-			e.Message = httpRes.Header.Get(StreamErrHeader)


Should we not be returning an error in this case?

As it stands now the error will be returned on the first call to Next(), but I can also make it return immediately. I believe that made the test case more complex, but maybe that's okay.

Stebalien · 2018-09-07T01:39:44Z

http/responseemitter.go

-		err = re.closeWithError(err)
+		// always close singles with nil error.
+		// encoding errors go to caller.
+		err = re.closeWithError(nil)


If we return the error to the "caller", expecting them to do something, we can't close. This should be something like isSingle && err != nil. Am I missing something?

It should have been isSingle && err == nil. If an encoding or copy error occurs, we didn't send the value so it doesn't make sense to close the emitter but we should return it. If encoding went fine, gracefully close the emitter.

Stebalien · 2018-09-07T01:49:04Z

You know, I think this is pretty much ready (modulo those two questions/issues). This is going to be awesome!

…= 200

keks · 2018-09-10T16:45:05Z

Hey @Stebalien, please CR :)

Stebalien · 2018-09-12T17:15:42Z

@keks all ready to go! I'll merge this as soon as ipfs/kubo#5035 is ready.

ghost assigned keks May 24, 2018

ghost added the status/in-progress In progress label May 24, 2018

keks mentioned this pull request May 24, 2018

update for the go-ipfs-cmds refactor ipfs/kubo#5035

Merged

keks force-pushed the refactor/bundle branch from 63232bf to f144d9c Compare May 25, 2018 12:01

keks requested a review from Stebalien May 25, 2018 12:02

whyrusleeping reviewed May 28, 2018

View reviewed changes

whyrusleeping reviewed May 31, 2018

View reviewed changes

whyrusleeping reviewed Jun 11, 2018

View reviewed changes

whyrusleeping reviewed Jun 13, 2018

View reviewed changes

Stebalien reviewed Jun 21, 2018

View reviewed changes

Stebalien mentioned this pull request Jun 26, 2018

MakeTypedEncoder breaks when using re.SetError #65

Closed

keks force-pushed the refactor/bundle branch from fab2a3a to 8522cfc Compare June 30, 2018 14:32

Stebalien requested changes Jul 2, 2018

View reviewed changes

Stebalien requested changes Jul 20, 2018

View reviewed changes

keks and others added 10 commits August 23, 2018 18:34

pretty imports, whitespace

e076a94

http: non-fatal test errors

570e715

http: log when we can't guess encoding from response headers

e381065

http: when closing with error, write both error value and header

0bf7d9b

- setting the error is moved into the preamble - we call the preamble before the err!=nil branch because we send to the conn in there

Revert "remove old racy test"

edded83

This reverts commit a011832. Do not remove tests that reveal race conditions.

fix racy test

30e8731

shadow err

http: don't log.Error when parsing resp with encType "text"

906ee24

consistently set error trailer again

0e8aad8

better logging, errors, fmt

4e484d2

gx publish 2.0beta2

029db85

keks force-pushed the refactor/bundle branch from 635c6f2 to 029db85 Compare August 23, 2018 17:08

http: send errors as trailer iff status is 200, else as values

e488a34

keks force-pushed the refactor/bundle branch from ba50fd9 to e488a34 Compare August 31, 2018 10:39

vet

a47f893

Stebalien requested changes Sep 7, 2018

View reviewed changes

keks added 2 commits September 7, 2018 13:12

fix single corner case

f15edaf

http.Send: immediately return errors sent by the server with status !…

75e2a0b

…= 200

Stebalien approved these changes Sep 12, 2018

View reviewed changes

Stebalien changed the title ~~[WIP] Bundle of changes~~ Bundle of changes Sep 12, 2018

Stebalien mentioned this pull request Sep 17, 2018

Should add Int64Option/Uint64Option? ipfs/go-ipfs-cmdkit#28

Closed

Stebalien merged commit c1f1b23 into master Sep 19, 2018

ghost removed the status/in-progress In progress label Sep 19, 2018

Stebalien deleted the refactor/bundle branch September 19, 2018 21:12

This was referenced Sep 19, 2018

Change PostRun signature #102

Closed

Let Run return an error to be emitted #97

Closed


		func (re *chanResponseEmitter) Emit(v interface{}) error {
		if e, ok := v.(*cmdkit.Error); ok {

		@@ -57,7 +61,15 @@ func (r chanResponse) Error() cmdkit.Error {
		return nil


		return nil

		log.Errorf("got an error: %v\n%s", err, debug.Stack())

		@@ -105,6 +132,12 @@ func (x executor) Execute(req Request, re ResponseEmitter, env Environment) (e
		}

Bundle of changes #112

Bundle of changes #112

Conversation

keks commented May 24, 2018 • edited Loading

keks commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keks commented May 31, 2018

keks commented May 31, 2018

whyrusleeping commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keks commented Jun 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Jun 21, 2018

keks commented Jun 29, 2018

keks commented Jun 30, 2018

keks commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keks commented May 24, 2018 •

edited

Loading