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.

Sign up for GitHub

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

feat: better stream handling #140

Merged
merged 10 commits into from
Dec 23, 2016
Merged

feat: better stream handling #140

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
730c5b9
feat: better stream handling
dignifiedquire Dec 20, 2016
e66a6e5
fix: make ci happy
dignifiedquire Dec 20, 2016
8d43043
fix: call `close` only once on process kill
haadcode Dec 20, 2016
7b09529
feat: replace subcommandante with execa
dignifiedquire Dec 20, 2016
f6fadfb
docs(api): add first pass
dignifiedquire Dec 21, 2016
53667fe
test: port subcomandante tests for our new exec
dignifiedquire Dec 21, 2016
59fdf47
fix: replace execa with subcomandante
dignifiedquire Dec 22, 2016
6b651fb
test: skip sighup test on ci
dignifiedquire Dec 22, 2016
1d30243
chore(deps): update ipfs-api
dignifiedquire Dec 22, 2016
7d104e1
fix: properly cleanup when stopping
dignifiedquire Dec 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,4 @@ build
# https://www.npmjs.org/doc/misc/npm-faq.html#should-i-check-my-node_modules-folder-into-git
node_modules

lib
dist
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ sudo: false
language: node_js
node_js:
- 4
- 5
- 6
- stable

# Make sure we have new NPM.
Expand Down
13 changes: 7 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@
"name": "ipfsd-ctl",
"version": "0.17.0",
"description": "simple controls for an ipfs node",
"main": "lib/index.js",
"jsxnext:main": "src/index.js",
"main": "src/index.js",
"scripts": {
"lint": "aegir-lint",
"coverage": "aegir-coverage",
"test": "aegir-test --env node",
"build": "aegir-build --env node",
"release": "aegir-release --env node",
"release-minor": "aegir-release --type minor --env node",
"release-major": "aegir-release --type major --env node",
Expand Down Expand Up @@ -45,19 +43,22 @@
],
"license": "MIT",
"dependencies": {
"async": "^2.1.4",
"bl": "^1.1.2",
"go-ipfs-dep": "0.4.4",
"ipfs-api": "^12.0.3",
"multiaddr": "^2.1.0",
"once": "^1.4.0",
"pump": "^1.0.2",
"rimraf": "^2.5.4",
"run-series": "^1.1.4",
"shutdown": "^0.2.4",
"subcomandante": "^1.0.5"
},
Copy link
Member

Choose a reason for hiding this comment

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

@jbenet invested a lot of time making sure we had a thing that would not leave zombie IPFS processes in user machines. It was what blocked #89 all this time.

Until we have comprehensive tests to make sure that you can spawn a bunch of daemons and make sure they don't become zombies, we can't move to execa. Historically, a way to test this was with station

If I understand correctly, migrating to execa is not a requirement for any particular feature.

Copy link
Member

Choose a reason for hiding this comment

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

As for the tests: #95

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how the burden of proof lies on the one changing it, as I haven't seen any proof subcomandante being as good as you say it is. There are tests for the cleanup behavior inside execa, which I wrote when implementing. I also did manual tests with execa inside reusing the testsuite from subcomandante so those cases all work.

So far all I heard was 'there were extensive tests' but I haven't seen or read what those actually were, so please tell which scenarios you are concerned about and I can add tests for them

Copy link
Member Author

Choose a reason for hiding this comment

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

@diasdavid I've ported the test suite from node-subcomandante, to run against the new src/exec.js to add a baseline of coverage to this module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Soo even though those tests pass fine, it seems thing are wonky right now. I have 5 ipfs instances running after a single npm test run :(

I will revert the switch to execa tomorrow.

"devDependencies": {
"aegir": "^9.2.1",
"aegir": "^9.3.0",
"chai": "^3.5.0",
"mkdirp": "^0.5.1",
"multihashes": "^0.3.1",
Copy link
Member

Choose a reason for hiding this comment

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

Is multihashes needed somewhere else? I don't see it being used in the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's used in the tests:https://github.com/ipfs/js-ipfsd-ctl/pull/140/files#diff-faa503f61f8addeb603095cc2f40c82eR103

"pre-commit": "^1.2.1"
},
"repository": {
Expand All @@ -72,4 +73,4 @@
"example": "examples",
"test": "test"
}
}
}
63 changes: 34 additions & 29 deletions src/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const fs = require('fs')
const run = require('subcomandante')
const series = require('run-series')
const async = require('async')
const ipfs = require('ipfs-api')
const multiaddr = require('multiaddr')
const rimraf = require('rimraf')
Expand All @@ -11,6 +11,7 @@ const path = require('path')
const join = path.join
const bl = require('bl')
const once = require('once')
const pump = require('pump')

const ipfsDefaultPath = findIpfsExecutable()

Expand All @@ -32,16 +33,21 @@ function findIpfsExecutable () {
}
}

function configureNode (node, conf, done) {
const keys = Object.keys(conf)
series(keys.map((key) => (cb) => {
const value = conf[key]
const env = {env: node.env}
function setConfigValue (node, key, value, callback) {
const c = run(
node.exec,
['config', key, value, '--json'],
{env: node.env}
)
callback = once(callback)
c.once('error', callback)
c.once('close', callback)
}

run(node.exec, ['config', key, '--json', JSON.stringify(value)], env)
.on('error', cb)
.on('end', cb)
}), done)
function configureNode (node, conf, callback) {
async.eachOfSeries(conf, (value, key, cb) => {
setConfigValue(node, key, JSON.stringify(value), cb)
}, callback)
}

// Consistent error handling
Expand Down Expand Up @@ -69,15 +75,16 @@ module.exports = class Node {
}

_run (args, envArg, done) {
run(this.exec, args, envArg)
.on('error', done)
.pipe(bl((err, result) => {
pump(
run(this.exec, args, envArg),
bl((err, result) => {
if (err) {
return done(err)
}

done(null, result.toString().trim())
}))
})
)
}

init (initOpts, done) {
Expand All @@ -93,10 +100,12 @@ module.exports = class Node {
this.env.IPFS_PATH = this.path
}

run(this.exec, ['init', '-b', keySize], {env: this.env})
.on('error', done)
.pipe(bl((err, buf) => {
if (err) return done(err)
pump(
run(this.exec, ['init', '-b', keySize], {env: this.env}),
bl((err, buf) => {
if (err) {
return done(err)
}

configureNode(this, this.opts, (err) => {
if (err) {
Expand All @@ -108,7 +117,8 @@ module.exports = class Node {

done(null, this)
})
}))
})
)

if (this.disposable) {
shutdown.addHandler('disposable', 1, this.shutdown.bind(this))
Expand All @@ -120,10 +130,7 @@ module.exports = class Node {
// something similar to "stopDaemon()". consider changing it. - @jbenet
shutdown (done) {
if (!this.clean && this.disposable) {
rimraf(this.path, (err) => {
if (err) throw err
done()
})
rimraf(this.path, done)
}
}

Expand All @@ -135,7 +142,9 @@ module.exports = class Node {

const node = this
parseConfig(node.path, (err, conf) => {
if (err) return done(err)
if (err) {
return done(err)
}

let stdout = ''
let args = ['daemon'].concat(flags || [])
Expand Down Expand Up @@ -255,11 +264,7 @@ module.exports = class Node {
}

setConfig (key, value, done) {
done = once(done)
run(this.exec, ['config', key, value, '--json'], {env: this.env})
.on('error', done)
.on('data', () => {})
.on('end', () => done())
setConfigValue(this, key, value, done)
}

replaceConf (file, done) {
Expand Down
Loading