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

Jump to bottom

Graceful stop #205

Merged
merged 25 commits into from
Mar 14, 2018
Merged

Graceful stop #205

Show file tree
Hide file tree
Changes from 22 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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
"detect-port": "^1.2.2",
"dirty-chai": "^2.0.1",
"go-ipfs-dep": "0.4.13",
"ipfs": "^0.27.5",
"ipfs": "~0.28.0",
"is-running": "1.0.5",
"mkdirp": "^0.5.1",
"pre-commit": "^1.2.2",
Expand Down
31 changes: 17 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,13 @@ 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.init((err) => {
if (err) { return callback(err) }
node.version(callback)
})
Copy link
Member

Choose a reason for hiding this comment

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

Is this starting the node? I'm sure this is causing repo lock just to check the version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @dryajov, curious about this as there is a TODO to fix this on the go/js factory. Did the mentioned solution hit roadblocks originally? Can we make an issue and work on it separate from this PR?

Copy link
Member

Choose a reason for hiding this comment

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

It is very odd to me that we start a new instance of IPFS to get the version. This will create a memory leak + it will lock the repo for a consequent node spawn. @dryajov can you expand why to do it this way?

Copy link
Member

Choose a reason for hiding this comment

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

@diasdavid I'm not aware of any other way to get the version - is there a better one you can suggest?

Copy link
Member

Choose a reason for hiding this comment

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

btw, the instance is not started, it is simply initialized with a repo, to start it we would have to call node.start.

Copy link
Member

Choose a reason for hiding this comment

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

@dryajov are you saying that a repo is created to just get the version of ipfs? Sound like the node.version should work offline/un'init for the info it can provide, namely, the impl version.

If it was absolutely impossible (which it isn't), at least there should be a cleanup step here.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I thought about just using ipfs-repo directly to read the version, but it turns out we still need to initialize a repo to get the version, since there is no guarantee that the default repo in ~/.jsipfs corresponds to the version of the IPFS in our exec param.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it turns out that the version is read by the repo here - https://github.com/ipfs/js-ipfs/blob/494da7f8c5b35f491f22a986ff5e8c456cc0e602/src/core/components/version.js#L14...L24. I think that's an overkill and we should separate the repo version from the ipfs version, that way we don't need a repo. If noone objects I can go ahead and make that change in IPFS.

Copy link
Member

Choose a reason for hiding this comment

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

There is a need for a Repo version and IPFS version, that is correct. It should be ok to get the IPFS version without the Repo existing though.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed - I'm making the change in IPFS to allow for that.

})
}

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

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

options.init = typeof options.init !== 'undefined'
? options.init
: true
Expand Down Expand Up @@ -129,15 +131,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