-
Notifications
You must be signed in to change notification settings - Fork 62
feat: better stream handling #140
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
Changes from all commits
730c5b9
e66a6e5
8d43043
7b09529
f6fadfb
53667fe
59fdf47
6b651fb
1d30243
7d104e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ sudo: false | |
language: node_js | ||
node_js: | ||
- 4 | ||
- 5 | ||
- 6 | ||
- stable | ||
|
||
# Make sure we have new NPM. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
'use strict' | ||
|
||
// Start a disposable node, and get access to the api | ||
// print the node id, and kill the temporary daemon | ||
|
||
// IPFS_PATH will point to /tmp/ipfs_***** and will be | ||
// cleaned up when the process exits. | ||
|
||
const ipfsd = require('ipfsd-ctl') | ||
|
||
ipfsd.disposableApi((err, ipfs) => { | ||
if (err) throw err | ||
ipfs.id((err, id) => { | ||
if (err) throw err | ||
console.log(id) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,16 +2,15 @@ | |
"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", | ||
"docs": "aegir-docs", | ||
"release": "aegir-release --env node --docs", | ||
"release-minor": "aegir-release --type minor --env node --docs", | ||
"release-major": "aegir-release --type major --env node --docs", | ||
"coverage-publish": "aegir-coverage publish" | ||
}, | ||
"engines": { | ||
|
@@ -45,20 +44,22 @@ | |
], | ||
"license": "MIT", | ||
"dependencies": { | ||
"bl": "^1.1.2", | ||
"async": "^2.1.4", | ||
"go-ipfs-dep": "0.4.4", | ||
"ipfs-api": "^12.0.3", | ||
"multiaddr": "^2.1.0", | ||
"ipfs-api": "^12.1.2", | ||
"multiaddr": "^2.1.1", | ||
"once": "^1.4.0", | ||
"rimraf": "^2.5.4", | ||
"run-series": "^1.1.4", | ||
"shutdown": "^0.2.4", | ||
"subcomandante": "^1.0.5" | ||
}, | ||
"devDependencies": { | ||
"aegir": "^9.2.1", | ||
"aegir": "^9.3.0", | ||
"chai": "^3.5.0", | ||
"is-running": "^2.1.0", | ||
"mkdirp": "^0.5.1", | ||
"pre-commit": "^1.2.1" | ||
"multihashes": "^0.3.1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"pre-commit": "^1.2.2" | ||
}, | ||
"repository": { | ||
"type": "git", | ||
|
@@ -72,4 +73,4 @@ | |
"example": "examples", | ||
"test": "test" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
'use strict' | ||
|
||
const run = require('subcomandante') | ||
const once = require('once') | ||
|
||
function exec (cmd, args, opts, handlers) { | ||
opts = opts || {} | ||
let err = '' | ||
let result = '' | ||
let callback | ||
|
||
// Handy method if we just want the result and err returned in a callback | ||
if (typeof handlers === 'function') { | ||
callback = once(handlers) | ||
handlers = { | ||
error: callback, | ||
data (data) { | ||
result += data | ||
}, | ||
done () { | ||
if (err) { | ||
return callback(new Error(err)) | ||
} | ||
callback(null, result.trim()) | ||
} | ||
} | ||
} | ||
|
||
// The listeners that will actually be set on the process | ||
const listeners = { | ||
data: handlers.data, | ||
error (data) { | ||
err += data | ||
}, | ||
done: once((code) => { | ||
if (typeof code === 'number' && code !== 0) { | ||
return handlers.error( | ||
new Error(`non-zero exit code ${code}\n | ||
while running: ${cmd} ${args.join(' ')}\n\n | ||
${err}`) | ||
) | ||
} | ||
if (handlers.done) { | ||
handlers.done() | ||
} | ||
}) | ||
} | ||
|
||
const command = run(cmd, args, opts) | ||
|
||
if (listeners.data) { | ||
command.stdout.on('data', listeners.data) | ||
} | ||
|
||
command.stderr.on('data', listeners.error) | ||
|
||
// If command fails to execute return directly to the handler | ||
command.on('error', handlers.error) | ||
|
||
command.on('close', listeners.done) | ||
command.on('exit', listeners.done) | ||
|
||
return command | ||
} | ||
|
||
module.exports = exec |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,66 +5,111 @@ const join = require('path').join | |
|
||
const Node = require('./node') | ||
|
||
const defaultOptions = { | ||
'Addresses.Swarm': ['/ip4/0.0.0.0/tcp/0'], | ||
'Addresses.Gateway': '', | ||
'Addresses.API': '/ip4/127.0.0.1/tcp/0', | ||
disposable: true, | ||
init: true | ||
} | ||
|
||
function tempDir () { | ||
return join(os.tmpdir(), `ipfs_${String(Math.random()).substr(2)}`) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the path to tmpDir available through some call? |
||
|
||
module.exports = { | ||
version (done) { | ||
(new Node()).version(done) | ||
/** | ||
* Control go-ipfs nodes directly from JavaScript. | ||
* | ||
* @namespace IpfsDaemonController | ||
*/ | ||
const IpfsDaemonController = { | ||
/** | ||
* Get the version of the currently used go-ipfs binary. | ||
* | ||
* @memberof IpfsDaemonController | ||
* @param {function(Error, string)} callback | ||
* @returns {undefined} | ||
*/ | ||
version (callback) { | ||
(new Node()).version(callback) | ||
}, | ||
local (path, opts, done) { | ||
/** | ||
* Create a new local node. | ||
* | ||
* @memberof IpfsDaemonController | ||
* @param {string} [path] - Location of the repo. Defaults to `$IPFS_PATH`, or `$HOME/.ipfs`, or `$USER_PROFILE/.ipfs`. | ||
* @param {Object} [opts={}] | ||
* @param {function(Error, Node)} callback | ||
* @returns {undefined} | ||
*/ | ||
local (path, opts, callback) { | ||
if (typeof opts === 'function') { | ||
done = opts | ||
callback = opts | ||
opts = {} | ||
} | ||
if (!done) { | ||
done = path | ||
if (!callback) { | ||
callback = path | ||
path = process.env.IPFS_PATH || | ||
join(process.env.HOME || | ||
process.env.USERPROFILE, '.ipfs') | ||
} | ||
process.nextTick(() => { | ||
done(null, new Node(path, opts)) | ||
callback(null, new Node(path, opts)) | ||
}) | ||
}, | ||
disposableApi (opts, done) { | ||
/** | ||
* Create a new disposable node and already start the daemon. | ||
* | ||
* @memberof IpfsDaemonController | ||
* @param {Object} [opts={}] | ||
* @param {function(Error, Node)} callback | ||
* @returns {undefined} | ||
*/ | ||
disposableApi (opts, callback) { | ||
if (typeof opts === 'function') { | ||
done = opts | ||
callback = opts | ||
opts = {} | ||
} | ||
this.disposable(opts, (err, node) => { | ||
if (err) return done(err) | ||
node.startDaemon(done) | ||
if (err) { | ||
return callback(err) | ||
} | ||
|
||
node.startDaemon(callback) | ||
}) | ||
}, | ||
disposable (opts, done) { | ||
/** | ||
* Create a new disposable node. | ||
* This means the repo is created in a temporary location and cleaned up on process exit. | ||
* | ||
* @memberof IpfsDaemonController | ||
* @param {Object} [opts={}] | ||
* @param {function(Error, Node)} callback | ||
* @returns {undefined} | ||
*/ | ||
disposable (opts, callback) { | ||
if (typeof opts === 'function') { | ||
done = opts | ||
callback = opts | ||
opts = {} | ||
} | ||
opts['Addresses.Swarm'] = ['/ip4/0.0.0.0/tcp/0'] | ||
opts['Addresses.Gateway'] = '' | ||
opts['Addresses.API'] = '/ip4/127.0.0.1/tcp/0' | ||
|
||
if (opts.apiAddr) { | ||
opts['Addresses.API'] = opts.apiAddr | ||
} | ||
let options = {} | ||
Object.assign(options, defaultOptions, opts || {}) | ||
|
||
if (opts.gatewayAddr) { | ||
opts['Addresses.Gateway'] = opts.gatewayAddr | ||
} | ||
const repoPath = options.repoPath || tempDir() | ||
const disposable = options.disposable | ||
delete options.disposable | ||
delete options.repoPath | ||
|
||
const node = new Node(opts.repoPath || tempDir(), opts, true) | ||
const node = new Node(repoPath, options, disposable) | ||
|
||
if (typeof opts.init === 'boolean' && opts.init === false) { | ||
process.nextTick(() => { | ||
done(null, node) | ||
}) | ||
if (typeof options.init === 'boolean' && | ||
options.init === false) { | ||
process.nextTick(() => callback(null, node)) | ||
} else { | ||
node.init((err) => { | ||
done(err, node) | ||
}) | ||
node.init((err) => callback(err, node)) | ||
} | ||
} | ||
} | ||
|
||
module.exports = IpfsDaemonController |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.