bugfixes and improvements (ipfs daemon flags) #107
Conversation
jbenet
force-pushed
the
fixes/startDaemon
branch
from
e1c0cd1
to
1dd63e8
Compare
- bug1: local var for the closure (this.subprocess set to null)
- bug2: order problem: should set .on('close') handler before .kill()
- now waits for "Daemon is ready" - also tracks Gateway address - checks failures on startup, and returns an error if so - this was a serious bug-- would not detect errors. - fails correctly, without leaving the process hanging
jbenet
force-pushed
the
fixes/startDaemon
branch
from
1dd63e8
to
349a44c
Compare
| @@ -44,7 +44,7 @@ | |||
| "license": "MIT", | |||
| "dependencies": { | |||
| "go-ipfs-dep": "0.4.1", | |||
| "ipfs-api": "^4.1.0", | |||
| "ipfs-api": "^9.0.0", | |||
| @@ -109,6 +106,8 @@ module.exports = class Node { | |||
| } | |||
|
|
|||
| // cleanup tmp files | |||
| // TODO: this is a bad name for a function. a user may call this expecting | |||
| // something similar to "stopDaemon()". consider changing it. - @jbenet | |||
| const apiM = stdout.match(/API server listening on (.*)\n/) | ||
| if (apiM) { | ||
| // found the API server listening. extract the addr. | ||
| node.apiAddr = apiM[1] |
There was a problem hiding this comment.
This is a bit ninja. the daemon leaves a 'api' file on the repo with the multiaddr that is listening. It would be more safe to use that than trusting that this string will be always the same.
There was a problem hiding this comment.
this was the way it was. i did not change it
There was a problem hiding this comment.
ACK. Opened a issue so that it doesn't get lost - #108
| const api = ipfs(node.apiAddr) | ||
| api.apiHost = addr.address | ||
| api.apiPort = addr.port | ||
| done2(null, api) |
There was a problem hiding this comment.
The js-ipfs-api is able to dial to multiaddrs, no need for parsing. It is a good practice to keep using multiaddrs for addrs everywhere
There was a problem hiding this comment.
this would change the expected api-- i dont necessarily want to break things expecting this to be here
| node.killProcess(() => {}) // we failed. kill, just to be sure... | ||
| } | ||
| done(err, val) | ||
| done = () => {} // in case it gets called twice |
There was a problem hiding this comment.
If we get multiple errors (queued) do we want to do node.killProcess several times? What about having a flag 'isDead'?
| @@ -118,51 +117,106 @@ module.exports = class Node { | |||
| } | |||
| } | |||
|
|
|||
| startDaemon (done) { | |||
| parseConfig(this.path, (err, conf) => { | |||
| startDaemon (flags, done) { | |||
There was a problem hiding this comment.
callbacks at the top level of the func should be callback just to keep being consistent with the rest of js ecosystem (I do notice that applies to other parts of this module, it just means it was never refactored)
There was a problem hiding this comment.
yeah i dont want to change this across whole module, that could be a separate PR
| .on('data', onData) | ||
|
|
||
| // done2 is called to call done after removing the event listeners | ||
| function done2 (err, val) { |
There was a problem hiding this comment.
This doesn't seem right. This function gets called on a successful start of a node or a on insuccessful one. Can we have instead a 'failure' func and a 'success' func instead?
There was a problem hiding this comment.
in both cases the same thing -- removing listeners -- has to happen, you would have two functions with the same code. this changes from the old behavior (which was just to call done, to this wrapper on done (done2, not the best name))
d74045f (jbenet, 2 minutes ago)
feat(startDeamon): allow passing flags to ipfs daemon
9c4dbce (jbenet, 4 minutes ago)
fix(startDaemon): fix the behavior of startDeamon
b523b1c (jbenet, 7 minutes ago)
fix: rm unused var (thanks, linter)
efa2aa4 (jbenet, 10 minutes ago)
note(shutdown): comment on problematic func name
d6ad480 (jbenet, 14 minutes ago)
fix(shutdown): fixed bugs in stopDaemon
3c50894 (jbenet, 17 minutes ago)
fix(tests): don't force changing test when src changes