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

Graceful stop #205

Merged
merged 25 commits into from
Mar 14, 2018
+68 −94
Merged

Graceful stop #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
00958a7
feat: use api/v0/shutdown to stop a deamon
richardschneider Feb 17, 2018
ec58831
feat: use api/v0/shutdown to stop a deamon
richardschneider Feb 17, 2018
1db2ae4
test: work on windows
richardschneider Feb 17, 2018
6ecc02a
test: timeout issues
richardschneider Feb 17, 2018
252e1d5
fix: timeout
richardschneider Feb 21, 2018
c3315ac
test: run on windows
richardschneider Feb 21, 2018
67a2041
chore: force a build
richardschneider Feb 23, 2018
8cd5abe
chore: update dependency on ipfs
richardschneider Mar 1, 2018
b1dc8ee
feat: use api/v0/shutdown to stop a deamon
richardschneider Feb 17, 2018
4f0c14d
test: work on windows
richardschneider Feb 17, 2018
3b32c03
test: timeout issues
richardschneider Feb 17, 2018
2aec828
feat: use api/v0/shutdown to stop a deamon
richardschneider Feb 17, 2018
22c4ae9
test: work on windows
richardschneider Feb 17, 2018
74f6dc8
fix: only use ipfs if its `ready`
dryajov Mar 8, 2018
a5d0ec5
fix: timeouts and conflicts
dryajov Mar 8, 2018
b3444f8
typo
dryajov Mar 8, 2018
162ffb8
fix: return callback on error
dryajov Mar 9, 2018
36fbb7a
fix: use this instead of self
dryajov Mar 9, 2018
5fc58bb
fix: correctly initialize proc nodes
dryajov Mar 13, 2018
b1f5538
fix: no need for setImmediate
dryajov Mar 13, 2018
a61fa50
fix: simplify version call
dryajov Mar 13, 2018
2d3cf40
fix: timeouts
dryajov Mar 13, 2018
3ffcee3
fix: no need to init repo to get version
dryajov Mar 13, 2018
4ad247b
fix: remove singleRun
dryajov Mar 13, 2018
36aa183
chore: update deps
daviddias Mar 14, 2018
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
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,6 @@ dist
docs

.idea

.nyc_output
.vscode
24 changes: 12 additions & 12 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,42 +74,42 @@
"license": "MIT",
"dependencies": {
"async": "^2.6.0",
"boom": "^7.1.1",
"boom": "^7.2.0",
"debug": "^3.1.0",
"detect-node": "^2.0.3",
"hapi": "^16.6.2",
"hat": "0.0.3",
"ipfs-api": "^18.1.1",
"ipfs-repo": "^0.18.5",
"joi": "^13.0.2",
"ipfs-api": "^18.1.2",
"ipfs-repo": "^0.18.7",
"joi": "^13.1.2",
"lodash.clone": "^4.5.0",
"lodash.defaults": "^4.2.0",
"lodash.defaultsdeep": "^4.6.0",
"multiaddr": "^3.0.2",
"once": "^1.4.0",
"readable-stream": "^2.3.3",
"readable-stream": "^2.3.5",
"rimraf": "^2.6.2",
"safe-json-parse": "^4.0.0",
"safe-json-stringify": "^1.0.4",
"safe-json-stringify": "^1.1.0",
"shutdown": "^0.3.0",
"stream-http": "^2.7.2",
"stream-http": "^2.8.0",
"subcomandante": "^1.0.5",
"superagent": "^3.8.2",
"truthy": "0.0.1"
},
"devDependencies": {
"aegir": "^12.4.0",
"aegir": "^13.0.6",
"chai": "^4.1.2",
"cross-env": "^5.1.3",
"cross-env": "^5.1.4",
"detect-port": "^1.2.2",
"dirty-chai": "^2.0.1",
"go-ipfs-dep": "0.4.13",
"ipfs": "^0.27.5",
"ipfs": "~0.28.2",
"is-running": "1.0.5",
"mkdirp": "^0.5.1",
"pre-commit": "^1.2.2",
"proxyquire": "^1.8.0",
"sinon": "^4.1.3",
"proxyquire": "^2.0.0",
"sinon": "^4.4.5",
"superagent-mocker": "^0.5.2",
"supertest": "^3.0.0"
},
Expand Down
28 changes: 14 additions & 14 deletions src/factory-in-proc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ class FactoryInProc {
callback = options
options = {}
}
const IPFS = this.exec
new IPFS(options).version((err, _version) => {
if (err) { callback(err) }
callback(null, _version)

const node = new Node(options)
node.once('ready', () => {
node.version(callback)
})
}

Expand Down Expand Up @@ -97,7 +97,6 @@ class FactoryInProc {
}

const options = defaults({}, opts, defaultOptions)

options.init = typeof options.init !== 'undefined'
? options.init
: true
Expand Down Expand Up @@ -129,15 +128,16 @@ class FactoryInProc {
}

const node = new Node(options)

series([
(cb) => options.init
? node.init(cb)
: cb(),
(cb) => options.start
? node.start(options.args, cb)
: cb()
], (err) => callback(err, node))
node.once('ready', () => {
series([
(cb) => options.init
? node.init(cb)
: cb(),
(cb) => options.start
? node.start(options.args, cb)
: cb()
], (err) => callback(err, node))
})
}
}

Expand Down
30 changes: 16 additions & 14 deletions src/ipfsd-daemon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,14 +148,13 @@ class Daemon {
return callback(err)
}

const self = this
waterfall([
(cb) => this.getConfig(cb),
(conf, cb) => this.replaceConfig(defaults({}, this.opts.config, conf), cb)
], (err) => {
if (err) { return callback(err) }
self.clean = false
self.initialized = true
this.clean = false
this.initialized = true
return callback()
})
})
Expand Down Expand Up @@ -270,8 +269,9 @@ class Daemon {
/**
* Kill the `ipfs daemon` process.
*
* First `SIGTERM` is sent, after 10.5 seconds `SIGKILL` is sent
* if the process hasn't exited yet.
* If the HTTP API is established, then send 'shutdown' command; otherwise,
* process.kill(`SIGTERM`) is used. In either case, if the process
* does not exit after 10.5 seconds then a `SIGKILL` is used.
*
* @param {function()} callback - Called when the process was killed.
* @returns {undefined}
Expand All @@ -282,24 +282,26 @@ class Daemon {
const timeout = setTimeout(() => {
log('kill timeout, using SIGKILL', subprocess.pid)
subprocess.kill('SIGKILL')
callback()
}, GRACE_PERIOD)

const disposable = this.disposable
const clean = this.cleanup.bind(this)
subprocess.once('close', () => {
subprocess.once('exit', () => {
log('killed', subprocess.pid)
clearTimeout(timeout)
this.subprocess = null
this._started = false
if (disposable) {
return clean(callback)
if (this.disposable) {
return this.cleanup(callback)
}
callback()
setImmediate(callback)
})

log('killing', subprocess.pid)
subprocess.kill('SIGTERM')
if (this.api) {
log('kill via api', subprocess.pid)
this.api.shutdown(() => null)
} else {
log('killing', subprocess.pid)
subprocess.kill('SIGTERM')
}
this.subprocess = null
}

Expand Down
10 changes: 8 additions & 2 deletions src/ipfsd-in-proc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@ const createRepo = require('./utils/repo/create-nodejs')
const defaults = require('lodash.defaults')
const waterfall = require('async/waterfall')
const debug = require('debug')
const EventEmitter = require('events')

const log = debug('ipfsd-ctl:in-proc')

let IPFS = null

/**
* ipfsd for a js-ipfs instance (aka in-process IPFS node)
*/
class Node {
class Node extends EventEmitter {
/**
* Create a new node.
*
Expand All @@ -21,9 +24,10 @@ class Node {
* @returns {Node}
*/
constructor (opts) {
super()
this.opts = opts || {}

const IPFS = this.opts.exec
IPFS = this.opts.exec

this.opts.args = this.opts.args || []
this.path = this.opts.repoPath
Expand Down Expand Up @@ -68,6 +72,8 @@ class Node {
EXPERIMENTAL: this.opts.EXPERIMENTAL,
libp2p: this.opts.libp2p
})

this.exec.once('ready', () => this.emit('ready'))
Copy link
Member

Choose a reason for hiding this comment

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

Why use a different pattern than the one used for ipfs-daemon (new Node + node.start)?

Copy link
Member

@dryajov dryajov Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I believe not doing that can end up in race conditions - take a look at maybeOpenRepo, its called by the IPFS constructor through boot(self), but it is async, and the only way of knowing when it finished is by hooking into the ready event.

Copy link
Member

Choose a reason for hiding this comment

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

What I'm weird'ed out is that in ipfsd-in-proc, the IPFS instance starts (and therefore does network activity, lock repo, etc) on the constructor, while in ipfsd-daemon, the daemon only starts when .start is called. This asymmetry will lead to confusion and debugging issues in the future.

Copy link
Member

Choose a reason for hiding this comment

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

ah, I see - we're not actually starting the node since IPFS is constructed with both, init and start as false, take a look here - https://github.com/ipfs/js-ipfsd-ctl/blob/a61fa50d6834e61465398cd1d54e60ec913358eb/src/ipfsd-in-proc.js#L67...L74. This was done specifically to keep that symmetry with the other types of daemons, the ready event is there to make sure we don't trip over anything.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Makes sense then. Thanks for clarifying :)

}

/**
Expand Down
2 changes: 1 addition & 1 deletion test/add-retrieve.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ describe('data can be put and fetched', () => {
let ipfsd

before(function (done) {
this.timeout(20 * 1000)
this.timeout(30 * 1000)

const f = IPFSFactory.create(dfOpts)

Expand Down
14 changes: 3 additions & 11 deletions test/api.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@ const series = require('async/series')
const multiaddr = require('multiaddr')
const path = require('path')
const DaemonFactory = require('../src')
const os = require('os')

const isNode = require('detect-node')
const isWindows = os.platform() === 'win32'

const tests = [
{ type: 'go' },
Expand All @@ -40,15 +37,10 @@ describe('ipfsd.api for Daemons', () => {
})

it('test the ipfsd.api', function (done) {
this.timeout(20 * 1000)
this.timeout(50 * 1000)

// TODO skip in browser - can we avoid using file system operations here?
if (!isNode) { this.skip() }

// TODO: fix on Windows
// - https://github.com/ipfs/js-ipfsd-ctl/pull/155#issuecomment-326970190
// - https://github.com/ipfs/js-ipfs-api/issues/408
if (isWindows) { return this.skip() }
if (!isNode) { return this.skip() }

let ipfsd
let api
Expand Down Expand Up @@ -112,7 +104,7 @@ describe('ipfsd.api for Daemons', () => {
})

it('check if API and Gateway addrs are correct', function (done) {
this.timeout(20 * 1000)
this.timeout(50 * 1000)

df.spawn({
config: config,
Expand Down
26 changes: 8 additions & 18 deletions test/spawn-options.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ const isNode = require('detect-node')
const hat = require('hat')
const IPFSFactory = require('../src')
const JSIPFS = require('ipfs')
const os = require('os')

const isWindows = os.platform() === 'win32'

const tests = [
{ type: 'go', bits: 1024 },
Expand All @@ -30,7 +27,9 @@ const versions = {
proc: jsVersion
}

describe('Spawn options', () => {
describe('Spawn options', function () {
this.timeout(20 * 1000)

tests.forEach((fOpts) => describe(`${fOpts.type}`, () => {
const VERSION_STRING = versions[fOpts.type]
let f
Expand All @@ -43,7 +42,7 @@ describe('Spawn options', () => {
it('f.version', function (done) {
this.timeout(20 * 1000)

f.version({ type: fOpts.type }, (err, version) => {
f.version({ type: fOpts.type, exec: fOpts.exec }, (err, version) => {
expect(err).to.not.exist()
if (fOpts.type === 'proc') { version = version.version }
expect(version).to.be.eql(VERSION_STRING)
Expand Down Expand Up @@ -109,7 +108,7 @@ describe('Spawn options', () => {
})

it('ipfsd.stop', function (done) {
this.timeout(10 * 1000)
this.timeout(20 * 1000)

ipfsd.stop(done)
})
Expand All @@ -124,9 +123,6 @@ describe('Spawn options', () => {
let ipfsd

it('f.spawn', function (done) {
// TODO: wont work on windows until we get `/shutdown` implemented in js-ipfs
if (isWindows) { this.skip() }

this.timeout(20 * 1000)

const options = {
Expand All @@ -147,9 +143,6 @@ describe('Spawn options', () => {
})

it('ipfsd.start', function (done) {
// TODO: wont work on windows until we get `/shutdown` implemented in js-ipfs
if (isWindows) { this.skip() }

this.timeout(20 * 1000)

ipfsd.start((err, api) => {
Expand All @@ -161,9 +154,6 @@ describe('Spawn options', () => {
})

it('ipfsd.stop', function (done) {
// TODO: wont work on windows until we get `/shutdown` implemented in js-ipfs
if (isWindows) { this.skip() }

this.timeout(20 * 1000)

ipfsd.stop(done)
Expand Down Expand Up @@ -199,7 +189,7 @@ describe('Spawn options', () => {

describe('custom config options', () => {
it('custom config', function (done) {
this.timeout(30 * 1000)
this.timeout(50 * 1000)

const addr = '/ip4/127.0.0.1/tcp/5678'
const swarmAddr1 = '/ip4/127.0.0.1/tcp/35666'
Expand Down Expand Up @@ -311,7 +301,7 @@ describe('Spawn options', () => {
let ipfsd

it('spawn with pubsub', function (done) {
this.timeout(30 * 1000)
this.timeout(20 * 1000)

const options = {
args: ['--enable-pubsub-experiment'],
Expand Down Expand Up @@ -344,7 +334,7 @@ describe('Spawn options', () => {
})

it('ipfsd.stop', function (done) {
this.timeout(10 * 1000)
this.timeout(20 * 1000)
ipfsd.stop(done)
})
})
Expand Down
Loading