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

Daemon code #44

Merged
merged 10 commits into from
Sep 14, 2014
Merged

Daemon code #44

merged 10 commits into from
Sep 14, 2014

Conversation

whyrusleeping
Copy link
Member

This pull request allows ipfs to run as a daemon that is communicated to via a TCP socket. When the daemon is running, all 'ipfs' commands will be run on the daemon process as opposed to the ipfs command process.

@jbenet
Copy link
Member

jbenet commented Sep 12, 2014

One general point on this PR is on the very simple low-level commands (add, ls, cat, refs, etc). I want to keep these as simple and independent as possible, like the git plumbing. So that people can use these commands in other scripts/programs. this means their output should be consistent across cli + api, and really easy to parse in cli. (organizationally, keeping them in separate files makes it easy for people to add more cmds, have help associated with them, etc. it's possible that we can expose the entire set of commands by writing a wrapper around go-commander, i.e. the new process serializes the cmd (as you're doing), sends it over, and the receiving process just launches a command in the go-commander way (initializing the node beforehand).

e.g.

> ipfs add -r foo
added QmXBfrvxa8mqiHvuD5jEDer7cH6b6Mb1ypZGAbMLogXtgi foo/bar
added QmUJvdmuykEBXfn4xZp1sFwvSh2PqiqieSEbJaAe4oBH94 foo

> ipfs ls QmUJvdmuykEBXfn4xZp1sFwvSh2PqiqieSEbJaAe4oBH94
QmXBfrvxa8mqiHvuD5jEDer7cH6b6Mb1ypZGAbMLogXtgi 6 bar

these can be built upon by other tools.

So making these commands very simple to write, and cohesive, for both cli and api would be ideal.
Maybe something where the RPC API ends up mirroring the output directly:

> POST http://localhost:4100/api -d {"command": "ls", "args": ["QmUJvdmuykEBXfn4xZp1sFwvSh2PqiqieSEbJaAe4oBH94"] }
{ "command": "ls", 
  "args": ["QmUJvdmuykEBXfn4xZp1sFwvSh2PqiqieSEbJaAe4oBH94"], 
  "output": [ ["QmXBfrvxa8mqiHvuD5jEDer7cH6b6Mb1ypZGAbMLogXtgi", 6, "bar"] ] }

Not sure.

ExecuteCommand(&command, dl.node, conn)
}

func ExecuteCommand(com *Command, ipfsnode *core.IpfsNode, out io.Writer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Go Way of handling errors is to try to be honest with callers about errors that arise during operation. The construction of large systems is simplified through the combination of (1) Systematic propagation of errors and (2) well-defined caller/callee contracts.

There's always some place where the buck stops, but I'm often cautious of silencing errors too early. Doing so deep in the call stack limits one's ability to handle exceptional control flow.

When a choice is made to handle an error rather than bubbling it up, it's important to have a principled reason for the course of action.

In this case, it looks like the error isn't returned because the system wishes to express specific messages about the nature of the failure. The language-provided mechanism for this is to instantiate an error with a detailed message. This error can be returned to the caller and the caller can choose what to do with it.

if err != nil {
  return errors.New("This is a really bad error")
}
return nil

https://golang.org/doc/effective_go.html#errors

Admittedly, there are alternate approaches (eg. exceptions in Python), but since the standard library takes such a hard stance in favor of error objects and multiple return values, it is often best to go with that flow rather than defy convention.

One caveat: In asynchronous contexts, it can sometimes be difficult to get return errors bubbled up to interested parties. In those cases, it often comes down to a well-considered judgement call.

Copy link
Member Author

Choose a reason for hiding this comment

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

@perfmode Im confused about what youre getting at here. In this case, the only error I deal with is printed back to the user.

Copy link
Member

Choose a reason for hiding this comment

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

maybe that it's on a different band? (i.e. the request is coming in through the network, and the error is printed to the console?)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the error is being printed back to wherever the command is coming from, which is stdout in the local case and the socket in the daemon case.

Copy link
Member

Choose a reason for hiding this comment

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

oh right conn :)

Copy link
Contributor

Choose a reason for hiding this comment

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

My comment is on the function signature, but GitHub has chosen to highlight the block of code above it. I'll try to demonstrate with a snippet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so youre saying ExecuteCommand should return an error? I think that would be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. to try--as much as possible--to allow errors to bubble up.

I'll try to be clearer with the snippets in the future.

@whyrusleeping
Copy link
Member Author

One idea i was toying with was somehow multiplexing the connection to allow for errors and standard out to be separated. Although im not sure how much value that would provide.

@whyrusleeping whyrusleeping added the status/in-progress In progress label Sep 12, 2014
@whyrusleeping whyrusleeping self-assigned this Sep 12, 2014
@whyrusleeping whyrusleeping added codereview and removed status/in-progress In progress labels Sep 13, 2014
@jbenet
Copy link
Member

jbenet commented Sep 14, 2014

@whyrusleeping this LGTM

@whyrusleeping whyrusleeping merged commit 35a87e9 into master Sep 14, 2014
@btc btc deleted the daemon branch September 14, 2014 09:42
ribasushi pushed a commit that referenced this pull request Jul 4, 2021
protect loggers with rwmutex
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
fix ticker leak in BootstrapWithConfig
longfeiWan9 pushed a commit to longfeiWan9/go-ipfs that referenced this pull request Nov 18, 2021
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Apr 7, 2022
fix: return ErrLinkNotFound when the _link_ isn't found
@ajnavarro ajnavarro mentioned this pull request Aug 24, 2022
72 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants