feat: graceful stop, fix stop for Windows (#205)

ipfs · Mar 14, 2018 · 359dd62 · 359dd62
1 parent 4cc69a0
commit 359dd62
Show file tree

Hide file tree

Showing 9 changed files with 68 additions and 94 deletions.
diff --git a/.gitignore b/.gitignore
@@ -38,3 +38,6 @@ dist
 docs
 
 .idea
+
+.nyc_output
+.vscode
diff --git a/package.json b/package.json
@@ -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"
   },

diff --git a/src/factory-in-proc.js b/src/factory-in-proc.js
@@ -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)
     })
   }
 
@@ -97,7 +97,6 @@ class FactoryInProc {
     }
 
     const options = defaults({}, opts, defaultOptions)
-
     options.init = typeof options.init !== 'undefined'
       ? options.init
       : true
@@ -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))
+    })
   }
 }
 

diff --git a/src/ipfsd-daemon.js b/src/ipfsd-daemon.js
@@ -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()
       })
     })
@@ -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}
@@ -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
   }
 

diff --git a/src/ipfsd-in-proc.js b/src/ipfsd-in-proc.js
@@ -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.
    *
@@ -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
@@ -68,6 +72,8 @@ class Node {
       EXPERIMENTAL: this.opts.EXPERIMENTAL,
       libp2p: this.opts.libp2p
     })
+
+    this.exec.once('ready', () => this.emit('ready'))
   }
 
   /**

diff --git a/test/add-retrieve.spec.js b/test/add-retrieve.spec.js
@@ -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)
 

diff --git a/test/api.spec.js b/test/api.spec.js
@@ -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' },
@@ -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
@@ -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,

diff --git a/test/spawn-options.spec.js b/test/spawn-options.spec.js
@@ -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 },
@@ -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
@@ -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)
@@ -109,7 +108,7 @@ describe('Spawn options', () => {
         })
 
         it('ipfsd.stop', function (done) {
-          this.timeout(10 * 1000)
+          this.timeout(20 * 1000)
 
           ipfsd.stop(done)
         })
@@ -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 = {
@@ -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) => {
@@ -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)
@@ -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'
@@ -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'],
@@ -344,7 +334,7 @@ describe('Spawn options', () => {
       })
 
       it('ipfsd.stop', function (done) {
-        this.timeout(10 * 1000)
+        this.timeout(20 * 1000)
         ipfsd.stop(done)
       })
     })