feat: dont call spawn cb more than once on error

circle.yml

@@ -1,7 +1,7 @@
 # Warning: This file is automatically synced from https://github.com/ipfs/ci-sync so if you want to change it, please change it there and ask someone to sync all repositories.
 machine:
   node:
-    version: stable
+    version: 8.11.1
 
 test:
   post:

package.json

@@ -24,7 +24,7 @@
     "joi": false,
     "stream": "readable-stream",
     "http": "stream-http",
-    "./src/utils/repo/create-nodejs.js": "./src/utils/repo/create-browser.js",
+    "./src/utils/repo/nodejs.js": "./src/utils/repo/browser.js",
     "./src/utils/exec.js": false,
     "./src/utils/find-ipfs-executable.js": false,
     "./src/utils/tmp-dir.js": "./src/utils/tmp-dir-browser.js",
@@ -83,21 +83,21 @@
     "boom": "^7.2.0",
     "debug": "^3.1.0",
     "detect-node": "^2.0.3",
+    "dexie": "^1.5.1",
     "hapi": "^16.6.2",
     "hat": "0.0.3",
     "ipfs-api": "^21.0.0",
-    "ipfs-repo": "^0.19.0",
     "joi": "^13.1.2",
     "lodash.clone": "^4.5.0",
     "lodash.defaults": "^4.2.0",
     "lodash.defaultsdeep": "^4.6.0",
-    "multiaddr": "^4.0.0",
+    "multiaddr": "^5.0.0",
     "once": "^1.4.0",
     "readable-stream": "^2.3.6",
     "rimraf": "^2.6.2",
     "safe-json-parse": "^4.0.0",
     "safe-json-stringify": "^1.1.0",
-    "shutdown": "^0.3.0",
+    "shutdown": "~0.3.0",
     "stream-http": "^2.8.1",
     "subcomandante": "^1.0.5",
     "superagent": "^3.8.2",
@@ -115,7 +115,7 @@
     "mkdirp": "^0.5.1",
     "proxyquire": "^2.0.1",
     "sinon": "^4.5.0",
-    "superagent-mocker": "^0.5.2",
+    "superagent-mocker": "~0.5.2",
     "supertest": "^3.0.0"
   },
   "repository": {
@@ -125,9 +125,5 @@
   "bugs": {
     "url": "https://github.com/ipfs/js-ipfsd-ctl/issues"
   },
-  "homepage": "https://github.com/ipfs/js-ipfsd-ctl",
-  "directories": {
-    "example": "examples",
-    "test": "test"
-  }
+  "homepage": "https://github.com/ipfs/js-ipfsd-ctl"
 }

src/factory-in-proc.js

@@ -5,6 +5,8 @@ const clone = require('lodash.clone')
 const series = require('async/series')
 const path = require('path')
 const tmpDir = require('./utils/tmp-dir')
+const once = require('once')
+const repoUtils = require('./utils/repo/nodejs')
 
 const Node = require('./ipfsd-in-proc')
 const defaultConfig = require('./defaults/config')
@@ -130,14 +132,19 @@ class FactoryInProc {
     }
 
     const node = new Node(options)
-    node.once('error', err => callback(err, node))
+    const callbackOnce = once((err) => {
+      if (err) {
+        return callback(err)
+      }
+      callback(null, node)
+    })
+    node.once('error', callbackOnce)
 
     series([
       (cb) => node.once('ready', cb),
-      (cb) => node.repo._isInitialized(err => {
-        // if err exists, repo failed to find config or the ipfs-repo package
-        // version is different than that of the existing repo.
-        node.initialized = !err
+      (cb) => repoUtils.repoExists(node.path, (err, initialized) => {
+        if (err) { return cb(err) }
+        node.initialized = initialized
         cb()
       }),
       (cb) => options.init
@@ -146,7 +153,7 @@ class FactoryInProc {
       (cb) => options.start
         ? node.start(options.args, cb)
         : cb()
-    ], (err) => callback(err, node))
+    ], callbackOnce)
   }
 }
 

src/ipfsd-in-proc.js

@@ -2,7 +2,7 @@
 
 const multiaddr = require('multiaddr')
 const defaultsDeep = require('lodash.defaultsdeep')
-const createRepo = require('./utils/repo/create-nodejs')
+const repoUtils = require('./utils/repo/nodejs')
 const defaults = require('lodash.defaults')
 const waterfall = require('async/waterfall')
 const debug = require('debug')
@@ -30,14 +30,14 @@ class Node extends EventEmitter {
     IPFS = this.opts.exec
 
     this.opts.args = this.opts.args || []
-    this.path = this.opts.repoPath
-    this.repo = createRepo(this.path)
+    this.path = this.opts.repoPath || repoUtils.createTempRepoPath()
     this.disposable = this.opts.disposable
     this.clean = true
     this._apiAddr = null
     this._gatewayAddr = null
     this._started = false
     this.api = null
+    this.initialized = false
     this.bits = this.opts.initOptions ? this.opts.initOptions.bits : null
 
     this.opts.EXPERIMENTAL = defaultsDeep({}, opts.EXPERIMENTAL, {
@@ -64,14 +64,16 @@ class Node extends EventEmitter {
     })
 
     this.exec = new IPFS({
-      repo: this.repo,
+      repo: this.path,
       init: false,
       start: false,
       pass: this.opts.pass,
       EXPERIMENTAL: this.opts.EXPERIMENTAL,
       libp2p: this.opts.libp2p
     })
 
+    // TODO: should this be wrapped in a process.nextTick(), for context:
+    // https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/#why-use-process-nexttick
     this.exec.once('error', err => this.emit('error', err))
     this.exec.once('ready', () => this.emit('ready'))
   }
@@ -176,7 +178,8 @@ class Node extends EventEmitter {
       return callback()
     }
 
-    this.repo.teardown(callback)
+    repoUtils.removeRepo(this.path)
+    callback()
   }
 
   /**

src/utils/repo/browser.js

@@ -0,0 +1,24 @@
+/* global self */
+'use strict'
+
+const hat = require('hat')
+const Dexie = require('dexie')
+
+exports.createTempRepoPath = function createTempPathRepo () {
+  return '/ipfs-' + hat()
+}
+
+exports.removeRepo = function removeRepo (repoPath) {
+  Dexie.delete(repoPath)
+}
+
+exports.repoExists = function repoExists (repoPath, cb) {
+  const db = new Dexie(repoPath)
+  db.open(repoPath)
+    .then((store) => {
+      const table = store.table(repoPath)
+      return table
+        .count((cnt) => cb(null, cnt > 0))
+        .catch(cb)
+    }).catch(cb)
+}

src/utils/repo/nodejs.js

@@ -0,0 +1,29 @@
+'use strict'
+
+const os = require('os')
+const path = require('path')
+const hat = require('hat')
+const rimraf = require('rimraf')
+const fs = require('fs')
+
+exports.removeRepo = function removeRepo (dir) {
+  try {
+    fs.accessSync(dir)
+  } catch (err) {
+    // Does not exist so all good
+    return
+  }
+
+  return rimraf.sync(dir)
+}
+
+exports.createTempRepoPath = function createTempRepo () {
+  return path.join(os.tmpdir(), '/ipfs-test-' + hat())
+}
+
+exports.repoExists = function (repoPath, cb) {
+  fs.access(`${repoPath}/config`, (err) => {
+    if (err) { return cb(null, false) }
+    cb(null, true)
+  })
+}

test/spawn-options.spec.js

@@ -8,11 +8,13 @@ const chai = require('chai')
 const dirtyChai = require('dirty-chai')
 const expect = chai.expect
 chai.use(dirtyChai)
+
 const fs = require('fs')
 const isNode = require('detect-node')
 const hat = require('hat')
 const IPFSFactory = require('../src')
 const JSIPFS = require('ipfs')
+const once = require('once')
 
 const tests = [
   { type: 'go', bits: 1024 },
@@ -160,7 +162,6 @@ describe('Spawn options', function () {
       })
     })
 
-    // TODO ?? What is this test trying to prove?
     describe('spawn a node and attach api', () => {
       let ipfsd
 
@@ -440,5 +441,35 @@ describe('Spawn options', function () {
         })
       })
     })
+
+    describe(`don't callback twice on error`, () => {
+      if (fOpts.type !== 'proc') { return }
+      it('spawn with error', (done) => {
+        this.timeout(20 * 1000)
+        // `once.strict` should throw if its called more than once
+        const callback = once.strict((err, ipfsd) => {
+          if (err) { return done(err) }
+
+          ipfsd.once('error', () => {}) // avoid EventEmitter throws
+
+          // Do an operation, just to make sure we're working
+          ipfsd.api.id((err) => {
+            if (err) {
+              return done(err)
+            }
+
+            // Do something to make stopping fail
+            ipfsd.exec._repo.close((err) => {
+              if (err) { return done(err) }
+              ipfsd.stop((err) => {
+                expect(err).to.exist()
+                done()
+              })
+            })
+          })
+        })
+        f.spawn(callback)
+      })
+    })
   }))
 })

test/utils.spec.js

@@ -13,9 +13,6 @@ const path = require('path')
 const flatten = require('../src/utils/flatten')
 const tempDir = require('../src/utils/tmp-dir')
 const findIpfsExecutable = require('../src/utils/find-ipfs-executable')
-const createRepo = require('../src/utils/repo/create-nodejs')
-
-const IPFSRepo = require('ipfs-repo')
 
 describe('utils', () => {
   describe('.flatten', () => {
@@ -63,25 +60,5 @@ describe('utils', () => {
         expect(fs.existsSync(execPath)).to.be.ok()
       })
     })
-
-    describe('.createRepo', () => {
-      let repo = null
-      let repoPath = tempDir()
-
-      it('should create repo', () => {
-        repo = createRepo(repoPath)
-        expect(repo).to.exist()
-        expect(repo).to.be.instanceOf(IPFSRepo)
-        expect(fs.existsSync(repoPath)).to.be.ok()
-      })
-
-      it('should cleanup repo', (done) => {
-        repo.teardown((err) => {
-          expect(err).to.not.exist()
-          expect(!fs.existsSync(repoPath)).to.be.ok()
-          done()
-        })
-      })
-    })
   }
 })