Adder: Make sure errors from addAllAndPin make it to the client

[kevina]

Before the error would be printed by the daemon but not make it to the
client.

License: MIT
Signed-off-by: Kevin Atkinson k@kevina.org

[kevina]

Not sure if this is the best way to go about fixing this. A better way would be to somehow trigger this case in PostRun:

            case <-req.Context().Done():
                res.SetError(req.Context().Err(), cmds.ErrNormal)
                return
            }

But that doesn't seam possible. However, it could be that I just don't understand the code well enough.

[whyrusleeping]

This LGTM. I wonder why the error returned there doesnt get propogated to the other side... It does so in other commands

[kevina]

@whyrusleeping The problem here is that the Run() function has already returned with non-error status. res.SetError is then called in a go routine. When run without the daemon online this seams to work okay, when run with the daemon the server does not know what to do with the error so it just logs it. There does not appear to be a way to retrieve the error status when run with the daemon online; however, I might be missing something.

Is there another command that does a res.SetError in a go routene? If so, let me know I and will figure out what the problem is in this case.

[whyrusleeping]

@kevina I think cat does, but i'm not 100% sure really.

[kevina]

@whyrusleeping, actually cat doesn't.

I found the problem. The server was sending an error response but the client code in commands/http/client.go was dropping it. Not sure how to fix it. The error is being dropped in readStreamedJson() (see https://github.com/ipfs/go-ipfs/blob/master/commands/http/client.go#L247) because it doesn't know what to do with it as it is launched in a goroutine (see https://github.com/ipfs/go-ipfs/blob/master/commands/http/client.go#L183). Not sure how to fix this.

My recommendation is to merge this and file a bug report for a correct fix, but there code doesn't need to be merged right away.

[whyrusleeping]

@kevina what if that readStreamedJson call took in the Result object, and when it errors, sets the error on the result object before closing its output channel? That might do the trick

[kevina]

@whyrusleeping, that seams to work. Have a look at 2d795b5. I am just not 100% sure why it works...

If you are happy with the new change I will close this pull request and create a new one with the readStreamedJson fix.

BTW: There are tests case for this #2634. I am not really sure how to create a test case for this outside of #2634 as I would have to create an error somehow and make sure it doesn't get dropped.

[whyrusleeping]

@kevina hrm... I guess i'm fine with waiting on the test cases for that one. Lets go ahead and close this in favor of the other change

[kevina]

Closing in favor of #3276.