feat: add timeout to stop and killProcess (#228)

* feat: add timeout to stop and killProcess

README.md

@@ -214,15 +214,17 @@ Start the daemon.
 `callback` is a function with the signature `function(err, ipfsApi)` that receives an instance of `ipfs-api` on success or an instance of `Error` on failure
 
 
-#### `ipfsd.stop([callback])`
+#### `ipfsd.stop([timeout, callback])`
 
 Stop the daemon.
 
-`callback` is a function with the signature `function(err)` callback - function that receives an instance of `Error` on failure
+`callback` is a function with the signature `function(err)` callback - function that receives an instance of `Error` on failure. Use timeout to specify the grace period in ms before hard stopping the daemon. Otherwise, a grace period of `10500` ms will be used for disposable nodes and `10500 * 3` ms for non disposable nodes.
 
-#### `ipfsd.killProcess([callback])`
+#### `ipfsd.killProcess([timeout, callback])`
 
-Kill the `ipfs daemon` process.
+Kill the `ipfs daemon` process. Use timeout to specify the grace period in ms before hard stopping the daemon. Otherwise, a grace period of `10500` ms will be used for disposable nodes and `10500 * 3` ms for non disposable nodes.
+
+Note: timeout is ignored for `proc` nodes
 
 First a `SIGTERM` is sent, after 10.5 seconds `SIGKILL` is sent if the process hasn't exited yet.
 

src/endpoint/routes.js

@@ -191,7 +191,8 @@ module.exports = (server) => {
     path: '/stop',
     handler: (request, reply) => {
       const id = request.query.id
-      nodes[id].stop((err) => {
+      const timeout = request.payload.timeout
+      nodes[id].stop(timeout, (err) => {
         if (err) {
           return reply(boom.badRequest(err))
         }
@@ -213,7 +214,8 @@ module.exports = (server) => {
     path: '/kill',
     handler: (request, reply) => {
       const id = request.query.id
-      nodes[id].killProcess((err) => {
+      const timeout = request.payload.timeout
+      nodes[id].killProcess(timeout, (err) => {
         if (err) {
           return reply(boom.badRequest(err))
         }

src/ipfsd-client.js

@@ -148,14 +148,21 @@ class DaemonClient {
   /**
    * Stop the daemon.
    *
+   * @param {integer|undefined} timeout - Grace period to wait before force stopping the node
    * @param {function(Error)} [cb]
    * @returns {undefined}
    */
-  stop (cb) {
+  stop (timeout, cb) {
+    if (typeof timeout === 'function') {
+      cb = timeout
+      timeout = undefined
+    }
+
     cb = cb || (() => {})
     request
       .post(`${this.baseUrl}/stop`)
       .query({ id: this._id })
+      .send({ timeout })
       .end((err) => {
         if (err) {
           return cb(new Error(err.response.body.message))
@@ -169,17 +176,24 @@ class DaemonClient {
   /**
    * Kill the `ipfs daemon` process.
    *
-   * First `SIGTERM` is sent, after 7.5 seconds `SIGKILL` is sent
+   * First `SIGTERM` is sent, after 10.5 seconds `SIGKILL` is sent
    * if the process hasn't exited yet.
    *
+   * @param {integer|undefined} timeout - Grace period to wait before force stopping the node
    * @param {function()} [cb] - Called when the process was killed.
    * @returns {undefined}
    */
-  killProcess (cb) {
+  killProcess (timeout, cb) {
+    if (typeof timeout === 'function') {
+      cb = timeout
+      timeout = undefined
+    }
+
     cb = cb || (() => {})
     request
       .post(`${this.baseUrl}/kill`)
       .query({ id: this._id })
+      .send({ timeout })
       .end((err) => {
         if (err) {
           return cb(new Error(err.response.body.message))

src/ipfsd-daemon.js

@@ -22,7 +22,11 @@ const findIpfsExecutable = require('./utils/find-ipfs-executable')
 const setConfigValue = require('./utils/set-config-value')
 const run = require('./utils/run')
 
-const GRACE_PERIOD = 10500 // amount of ms to wait before sigkill
+// amount of ms to wait before sigkill
+const GRACE_PERIOD = 10500
+
+// amount of ms to wait before sigkill for non disposable repos
+const NON_DISPOSABLE_GRACE_PERIOD = 10500 * 3
 
 /**
  * ipfsd for a go-ipfs or js-ipfs daemon
@@ -284,17 +288,23 @@ class Daemon {
   /**
    * Stop the daemon.
    *
+   * @param {integer|undefined} timeout - Grace period to wait before force stopping the node
    * @param {function(Error)} callback
    * @returns {undefined}
    */
-  stop (callback) {
+  stop (timeout, callback) {
+    if (typeof timeout === 'function') {
+      callback = timeout
+      timeout = null
+    }
+
     callback = callback || function noop () {}
 
     if (!this.subprocess) {
       return callback()
     }
 
-    this.killProcess(callback)
+    this.killProcess(timeout, callback)
   }
 
   /**
@@ -304,20 +314,32 @@ class Daemon {
    * process.kill(`SIGTERM`) is used.  In either case, if the process
    * does not exit after 10.5 seconds then a `SIGKILL` is used.
    *
+   * @param {integer|undefined} timeout - Grace period to wait before force stopping the node
    * @param {function()} callback - Called when the process was killed.
    * @returns {undefined}
    */
-  killProcess (callback) {
+  killProcess (timeout, callback) {
+    if (typeof timeout === 'function') {
+      callback = timeout
+      timeout = null
+    }
+
+    if (!timeout) {
+      timeout = this.disposable
+        ? GRACE_PERIOD
+        : NON_DISPOSABLE_GRACE_PERIOD
+    }
+
     // need a local var for the closure, as we clear the var.
     const subprocess = this.subprocess
-    const timeout = setTimeout(() => {
+    const grace = setTimeout(() => {
       log('kill timeout, using SIGKILL', subprocess.pid)
       subprocess.kill('SIGKILL')
-    }, GRACE_PERIOD)
+    }, timeout)
 
     subprocess.once('exit', () => {
       log('killed', subprocess.pid)
-      clearTimeout(timeout)
+      clearTimeout(grace)
       this.subprocess = null
       this._started = false
       if (this.disposable) {

test/endpoint/client.js

@@ -267,6 +267,48 @@ describe('client', () => {
     })
   })
 
+  describe('.stop with timeout', () => {
+    describe('handle valid', () => {
+      after(() => {
+        mock.clearRoutes()
+      })
+
+      it('should handle valid request', (done) => {
+        mock.post('http://localhost:9999/stop', (req) => {
+          expect(req.query.id).to.exist()
+        })
+
+        node.stop(1000, (err) => {
+          expect(err).to.not.exist()
+          done()
+        })
+      })
+    })
+
+    describe('handle invalid', () => {
+      after(() => {
+        mock.clearRoutes()
+      })
+
+      it('should handle invalid request', (done) => {
+        mock.post('http://localhost:9999/stop', () => {
+          const badReq = boom.badRequest()
+          return {
+            status: badReq.output.statusCode,
+            body: {
+              message: badReq.message
+            }
+          }
+        })
+
+        node.stop((err) => {
+          expect(err).to.exist()
+          done()
+        })
+      })
+    })
+  })
+
   describe('.killProcess', () => {
     describe('handle valid', () => {
       after(() => {

test/endpoint/routes.js

@@ -38,9 +38,9 @@ const routes = proxyquire(
           cb(null, api)
         }
 
-        node.stop = (cb) => node.killProcess(cb)
+        node.stop = (timeout, cb) => node.killProcess(timeout, cb)
 
-        node.killProcess = (cb) => {
+        node.killProcess = (timeout, cb) => {
           node.started = false
           cb()
         }
@@ -213,7 +213,7 @@ describe('routes', () => {
   })
 
   describe('POST /stop', () => {
-    it('should return 200', (done) => {
+    it('should return 200 without timeout', (done) => {
       server.inject({
         method: 'POST',
         url: `/stop?id=${id}`,
@@ -225,6 +225,18 @@ describe('routes', () => {
       })
     })
 
+    it('should return 200 with timeout', (done) => {
+      server.inject({
+        method: 'POST',
+        url: `/stop?id=${id}`,
+        headers: { 'content-type': 'application/json' },
+        payload: { id, timeout: 1000 }
+      }, (res) => {
+        expect(res.statusCode).to.equal(200)
+        done()
+      })
+    })
+
     it('should return 400', (done) => {
       server.inject({
         method: 'POST',
@@ -250,6 +262,18 @@ describe('routes', () => {
       })
     })
 
+    it('should return 200 with timeout', (done) => {
+      server.inject({
+        method: 'POST',
+        url: `/kill?id=${id}`,
+        headers: { 'content-type': 'application/json' },
+        payload: { id, timeout: 1000 }
+      }, (res) => {
+        expect(res.statusCode).to.equal(200)
+        done()
+      })
+    })
+
     it('should return 400', (done) => {
       server.inject({
         method: 'POST',

test/start-stop.node.js

@@ -149,6 +149,118 @@ tests.forEach((fOpts) => {
       })
     })
 
+    describe('start and stop with timeout', () => {
+      let ipfsd
+      let repoPath
+      let api
+      let pid
+      let stopped = false
+
+      before(function (done) {
+        this.timeout(50 * 1000)
+
+        const f = IPFSFactory.create(dfConfig)
+
+        f.spawn({
+          init: true,
+          start: false,
+          disposable: true,
+          initOptions: { bits: fOpts.bits }
+        }, (err, _ipfsd) => {
+          expect(err).to.not.exist()
+          expect(_ipfsd).to.exist()
+
+          ipfsd = _ipfsd
+          repoPath = ipfsd.path
+          done()
+        })
+      })
+
+      it('should return a node', () => {
+        expect(ipfsd).to.exist()
+      })
+
+      it('daemon exec path should match type', () => {
+        expect(exec).to.include.string(ipfsd.exec)
+      })
+
+      it('daemon should not be running', (done) => {
+        ipfsd.pid((pid) => {
+          expect(pid).to.not.exist()
+          done()
+        })
+      })
+
+      it('.start', function (done) {
+        this.timeout(20 * 1000)
+
+        ipfsd.start((err, ipfs) => {
+          expect(err).to.not.exist()
+
+          ipfsd.pid((_pid) => {
+            pid = _pid
+            api = ipfs
+
+            expect(isrunning(pid)).to.be.ok()
+            done()
+          })
+        })
+      })
+
+      it('is running', () => {
+        expect(api.id).to.exist()
+      })
+
+      it('.stop with timeout', function (done) {
+        this.timeout(15000 + 10) // should not take longer than timeout
+        ipfsd.stop(15000, (err) => {
+          expect(err).to.not.exist()
+          stopped = !isrunning(pid)
+          expect(stopped).to.be.ok()
+          done()
+        })
+      })
+
+      it('is stopped', function (done) {
+        // shutdown grace period is already 10500
+        this.timeout(20 * 1000)
+
+        ipfsd.pid((pid) => {
+          expect(pid).to.not.exist()
+          expect(stopped).to.equal(true)
+          expect(fs.existsSync(path.join(ipfsd.path, 'repo.lock'))).to.not.be.ok()
+          expect(fs.existsSync(path.join(ipfsd.path, 'api'))).to.not.be.ok()
+          done()
+        })
+      })
+
+      it('repo should cleaned up', () => {
+        expect(fs.existsSync(repoPath)).to.not.be.ok()
+      })
+
+      it('fail on start with non supported flags', function (done) {
+        // TODO js-ipfs doesn't fail on unrecognized args.
+        // Decided what should be the desired behaviour
+        if (fOpts.type === 'js') { return this.skip() }
+
+        const df = IPFSFactory.create(dfConfig)
+
+        df.spawn({
+          start: false,
+          initOptions: { bits: fOpts.bits }
+        }, (err, ipfsd) => {
+          expect(err).to.not.exist()
+          ipfsd.start(['--should-not-exist'], (err) => {
+            expect(err).to.exist()
+            expect(err.message)
+              .to.match(/unknown option "should-not-exist"\n/)
+
+            done()
+          })
+        })
+      })
+    })
+
     describe('start and stop with custom exec path', () => {
       let ipfsd
       before(function (done) {