Skip to content

Graceful stop #205

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

Merged
daviddias merged 25 commits into from
Mar 14, 2018
Merged

Graceful stop #205

Show file tree
Hide file tree
Changes from 16 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
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: 18 additions & 13 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,14 @@ class FactoryInProc {
callback = options
options = {}
}
const IPFS = this.exec
new IPFS(options).version((err, _version) => {
if (err) { callback(err) }
callback(null, _version)

const IPFS = this.exec // otherwise lint barfs
const ipfs = new IPFS(options)
ipfs.once('ready', () => {
ipfs.version((err, _version) => {
if (err) { callback(err) }
Copy link
Member

@daviddias daviddias Mar 9, 2018

Choose a reason for hiding this comment

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

needs a return. Otherwise chaos will happen.

Copy link
Contributor Author

@richardschneider richardschneider Mar 9, 2018

Choose a reason for hiding this comment

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

Agree, extremely bad code!!!

callback(null, _version)
})
Copy link
Member

@daviddias daviddias Mar 11, 2018

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

@JonKrone JonKrone Mar 12, 2018

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

@daviddias daviddias Mar 13, 2018

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

@dryajov dryajov Mar 13, 2018

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

@dryajov dryajov Mar 13, 2018

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

@daviddias daviddias Mar 13, 2018

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

@dryajov dryajov Mar 13, 2018

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

@dryajov dryajov Mar 13, 2018

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

@daviddias daviddias Mar 13, 2018

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

@dryajov dryajov Mar 13, 2018

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 @@ -129,15 +133,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.exec.once('ready', () => {
Copy link
Member

@daviddias daviddias Mar 11, 2018

Choose a reason for hiding this comment

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

Something isn't quite right here. the node is created but then what we listen on the ready event is exec??

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: 17 additions & 13 deletions src/ipfsd-daemon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,36 +270,40 @@ 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}
*/
killProcess (callback) {
// need a local var for the closure, as we clear the var.
const self = this
Copy link
Member

@daviddias daviddias Mar 9, 2018

Choose a reason for hiding this comment

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

Self is not needed.

Copy link
Contributor Author

@richardschneider richardschneider Mar 9, 2018

Choose a reason for hiding this comment

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

Disagree, it is needed in subprocess.once('exit', ...

this should not be used in a lambdaexpression! See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

const subprocess = this.subprocess
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)
self.subprocess = null
self._started = false
if (self.disposable) {
return self.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
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
10 changes: 1 addition & 9 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 Down Expand Up @@ -43,12 +40,7 @@ describe('ipfsd.api for Daemons', () => {
this.timeout(20 * 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
24 changes: 7 additions & 17 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 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(20 * 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
21 changes: 1 addition & 20 deletions test/start-stop.node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@ chai.use(dirtyChai)
const async = require('async')
const fs = require('fs')
const path = require('path')
const os = require('os')
const isrunning = require('is-running')

const isWindows = os.platform() === 'win32'
const findIpfsExecutable = require('../src/utils/find-ipfs-executable')
const tempDir = require('../src/utils/tmp-dir')
const IPFSFactory = require('../src')
Expand All @@ -35,8 +33,6 @@ tests.forEach((fOpts) => {
const dfConfig = Object.assign({}, dfBaseConfig, { type: fOpts.type })

describe('start and stop', () => {
if (isWindows) { return }

let ipfsd
let repoPath
let api
Expand Down Expand Up @@ -70,7 +66,7 @@ tests.forEach((fOpts) => {
it('daemon exec path should match type', () => {
let execPath = exec[fOpts.type]

expect(ipfsd.exec).to.include.string(execPath)
expect(ipfsd.exec).to.include.string(path.join(execPath))
})

it('daemon should not be running', (done) => {
Expand Down Expand Up @@ -260,26 +256,17 @@ tests.forEach((fOpts) => {
})

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

expect(ipfsd).to.exist()
})

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

ipfsd.pid((pid) => {
expect(pid).to.exist()
done()
})
})

it('.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((err) => {
Expand All @@ -292,9 +279,6 @@ tests.forEach((fOpts) => {
})

it('.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) => {
Expand All @@ -307,9 +291,6 @@ tests.forEach((fOpts) => {
})

it('.stop and cleanup', 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((err) => {
Expand Down