fix: swallowed errors

The switch to using `yargs-promise` for `ipfs init` and `ipfs daemon` commands caused an unhandled promise rejection and in some cases would cause an error to not be printed to the console.

This PR greatly simplifies the code in `src/cli/bin.js`, to always use `yargs-promise`. Command handlers are now passed an async `getIpfs` function instead of an `ipfs` instance. It means that we don't have to differentiate between commands that use an IPFS instance in `src/cli/bin.js`, giving the handler the power to call `getIpfs` or not to obtain an IPFS instance as and when needed. This removes a whole bunch of complexity from `src/cli/bin.js` at the cost of adding a single line to every command handler that needs to use an IPFS instance.

This enables operations like `echo "hello" | jsipfs add -q | jsipfs cid base32` to work without `jsipfs cid base32` failing because it's trying to acquire a repo lock when it doesn't use IPFS at all.

fixes #1835
refs #1858
refs libp2p/js-libp2p#311

License: MIT
Signed-off-by: Alan Shaw <[email protected]>

package.json

@@ -110,7 +110,7 @@
     "ipfs-block-service": "~0.15.1",
     "ipfs-http-client": "^29.0.0",
     "ipfs-http-response": "~0.2.1",
-    "ipfs-mfs": "~0.8.0",
+    "ipfs-mfs": "github:ipfs/js-ipfs-mfs#refactor/getIpfs",
     "ipfs-multipart": "~0.1.0",
     "ipfs-repo": "~0.26.1",
     "ipfs-unixfs": "~0.1.16",
@@ -168,7 +168,6 @@
     "pull-stream": "^3.6.9",
     "pull-stream-to-stream": "^1.3.4",
     "pump": "^3.0.0",
-    "read-pkg-up": "^4.0.0",
     "readable-stream": "^3.1.1",
     "receptacle": "^1.3.2",
     "stream-to-pull-stream": "^1.7.2",

src/cli/bin.js

@@ -5,118 +5,91 @@
 const YargsPromise = require('yargs-promise')
 const yargs = require('yargs')
 const updateNotifier = require('update-notifier')
-const readPkgUp = require('read-pkg-up')
 const utils = require('./utils')
 const print = utils.print
 const mfs = require('ipfs-mfs/cli')
 const debug = require('debug')('ipfs:cli')
+const pkg = require('../../package.json')
+
+async function main (args) {
+  const oneWeek = 1000 * 60 * 60 * 24 * 7
+  updateNotifier({ pkg, updateCheckInterval: oneWeek }).notify()
+
+  const cli = yargs
+    .option('silent', {
+      desc: 'Write no output',
+      type: 'boolean',
+      default: false,
+      coerce: silent => {
+        if (silent) utils.disablePrinting()
+        return silent
+      }
+    })
+    .option('pass', {
+      desc: 'Pass phrase for the keys',
+      type: 'string',
+      default: ''
+    })
+    .epilog(utils.ipfsPathHelp)
+    .demandCommand(1)
+    .fail((msg, err, yargs) => {
+      if (err) {
+        throw err // preserve stack
+      }
 
-const pkg = readPkgUp.sync({ cwd: __dirname }).pkg
-updateNotifier({
-  pkg,
-  updateCheckInterval: 1000 * 60 * 60 * 24 * 7 // 1 week
-}).notify()
-
-const args = process.argv.slice(2)
-
-const cli = yargs
-  .option('silent', {
-    desc: 'Write no output',
-    type: 'boolean',
-    default: false,
-    coerce: ('silent', silent => {
-      if (silent) {
-        utils.disablePrinting()
+      if (args.length > 0) {
+        print(msg)
       }
-      return silent
+
+      yargs.showHelp()
     })
-  })
-  .option('pass', {
-    desc: 'Pass phrase for the keys',
-    type: 'string',
-    default: ''
-  })
-  .epilog(utils.ipfsPathHelp)
-  .demandCommand(1)
-  .fail((msg, err, yargs) => {
-    if (err) {
-      throw err // preserve stack
-    }
 
-    if (args.length > 0) {
-      print(msg)
-    }
+  // Function to get hold of a singleton ipfs instance
+  const getIpfs = utils.singleton(cb => utils.getIPFS(yargs.argv, cb))
 
-    yargs.showHelp()
-  })
+  // add MFS (Files API) commands
+  mfs(cli)
 
-// Need to skip to avoid locking as these commands
-// don't require a daemon
-if (args[0] === 'daemon' || args[0] === 'init') {
   cli
+    .commandDir('commands')
     .help()
     .strict()
     .completion()
-    .command(require('./commands/daemon'))
-    .command(require('./commands/init'))
 
-  new YargsPromise(cli).parse(args)
-    .then(({ data }) => {
-      if (data) print(data)
-    })
-} else {
-  // here we have to make a separate yargs instance with
-  // only the `api` option because we need this before doing
-  // the final yargs parse where the command handler is invoked..
-  yargs().option('api').parse(process.argv, (err, argv, output) => {
-    if (err) {
-      throw err
+  let exitCode = 0
+
+  try {
+    const { data } = await new YargsPromise(cli, { getIpfs }).parse(args)
+    if (data) print(data)
+  } catch (err) {
+    debug(err)
+
+    // the argument can have a different shape depending on where the error came from
+    if (err.message) {
+      print(err.message)
+    } else if (err.error && err.error.message) {
+      print(err.error.message)
+    } else {
+      print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output')
     }
 
-    utils.getIPFS(argv, (err, ipfs, cleanup) => {
-      if (err) {
-        throw err
+    exitCode = 1
+  } finally {
+    // If an IPFS instance was used in the handler then clean it up here
+    if (getIpfs.instance) {
+      try {
+        const cleanup = getIpfs.rest[0]
+        await cleanup()
+      } catch (err) {
+        debug(err)
+        exitCode = 1
       }
+    }
+  }
 
-      // add MFS (Files API) commands
-      mfs(cli)
-
-      cli
-        .commandDir('commands')
-        .help()
-        .strict()
-        .completion()
-
-      let exitCode = 0
-
-      const parser = new YargsPromise(cli, { ipfs })
-      parser.parse(args)
-        .then(({ data, argv }) => {
-          if (data) {
-            print(data)
-          }
-        })
-        .catch((arg) => {
-          debug(arg)
-
-          // the argument can have a different shape depending on where the error came from
-          if (arg.message) {
-            print(arg.message)
-          } else if (arg.error && arg.error.message) {
-            print(arg.error.message)
-          } else {
-            print('Unknown error, please re-run the command with DEBUG=ipfs:cli to see debug output')
-          }
-
-          exitCode = 1
-        })
-        .then(() => cleanup())
-        .catch(() => {})
-        .then(() => {
-          if (exitCode !== 0) {
-            process.exit(exitCode)
-          }
-        })
-    })
-  })
+  if (exitCode) {
+    process.exit(exitCode)
+  }
 }
+
+main(process.argv.slice(2))

src/cli/commands/add.js

@@ -145,7 +145,7 @@ module.exports = {
 
   handler (argv) {
     argv.resolve((async () => {
-      const { ipfs } = argv
+      const ipfs = await argv.getIpfs()
       const options = {
         strategy: argv.trickle ? 'trickle' : 'balanced',
         shardSplitThreshold: argv.enableShardingExperiment

src/cli/commands/bitswap/stat.js

@@ -17,8 +17,9 @@ module.exports = {
     }
   },
 
-  handler ({ ipfs, cidBase, resolve }) {
+  handler ({ getIpfs, cidBase, resolve }) {
     resolve((async () => {
+      const ipfs = await getIpfs()
       const stats = await ipfs.bitswap.stat()
       stats.wantlist = stats.wantlist.map(k => cidToString(k['/'], { base: cidBase, upgrade: false }))
       stats.peers = stats.peers || []

src/cli/commands/bitswap/unwant.js

@@ -21,8 +21,9 @@ module.exports = {
       choices: multibase.names
     }
   },
-  handler ({ ipfs, key, cidBase, resolve }) {
+  handler ({ getIpfs, key, cidBase, resolve }) {
     resolve((async () => {
+      const ipfs = await getIpfs()
       await ipfs.bitswap.unwant(key)
       print(`Key ${cidToString(key, { base: cidBase, upgrade: false })} removed from wantlist`)
     })())

src/cli/commands/bitswap/wantlist.js

@@ -22,8 +22,9 @@ module.exports = {
     }
   },
 
-  handler ({ ipfs, peer, cidBase, resolve }) {
+  handler ({ getIpfs, peer, cidBase, resolve }) {
     resolve((async () => {
+      const ipfs = await getIpfs()
       const list = await ipfs.bitswap.wantlist(peer)
       list.Keys.forEach(k => print(cidToString(k['/'], { base: cidBase, upgrade: false })))
     })())

src/cli/commands/block/get.js

@@ -9,8 +9,9 @@ module.exports = {
 
   builder: {},
 
-  handler ({ ipfs, key, resolve }) {
+  handler ({ getIpfs, key, resolve }) {
     resolve((async () => {
+      const ipfs = await getIpfs()
       const block = await ipfs.block.get(key)
       if (block) {
         print(block.data, false)

src/cli/commands/block/put.js

@@ -52,7 +52,8 @@ module.exports = {
         })
       }
 
-      const { cid } = await argv.ipfs.block.put(data, argv)
+      const ipfs = await argv.getIpfs()
+      const { cid } = await ipfs.block.put(data, argv)
       print(cidToString(cid, { base: argv.cidBase }))
     })())
   }

src/cli/commands/block/rm.js

@@ -9,13 +9,14 @@ module.exports = {
 
   builder: {},
 
-  handler ({ ipfs, key, resolve }) {
+  handler ({ getIpfs, key, resolve }) {
     resolve((async () => {
       if (isDaemonOn()) {
         // TODO implement this once `js-ipfs-http-client` supports it
         throw new Error('rm block with daemon running is not yet implemented')
       }
 
+      const ipfs = await getIpfs()
       await ipfs.block.rm(key)
       print('removed ' + key)
     })())

src/cli/commands/block/stat.js

@@ -17,8 +17,9 @@ module.exports = {
     }
   },
 
-  handler ({ ipfs, key, cidBase, resolve }) {
+  handler ({ getIpfs, key, cidBase, resolve }) {
     resolve((async () => {
+      const ipfs = await getIpfs()
       const stats = await ipfs.block.stat(key)
       print('Key: ' + cidToString(stats.key, { base: cidBase }))
       print('Size: ' + stats.size)

src/cli/commands/bootstrap/add.js

@@ -17,7 +17,8 @@ module.exports = {
 
   handler (argv) {
     argv.resolve((async () => {
-      const list = await argv.ipfs.bootstrap.add(argv.peer, { default: argv.default })
+      const ipfs = await argv.getIpfs()
+      const list = await ipfs.bootstrap.add(argv.peer, { default: argv.default })
       list.Peers.forEach((peer) => print(peer))
     })())
   }

src/cli/commands/bootstrap/list.js

@@ -11,7 +11,8 @@ module.exports = {
 
   handler (argv) {
     argv.resolve((async () => {
-      const list = await argv.ipfs.bootstrap.list()
+      const ipfs = await argv.getIpfs()
+      const list = await ipfs.bootstrap.list()
       list.Peers.forEach((node) => print(node))
     })())
   }

src/cli/commands/bootstrap/rm.js

@@ -20,7 +20,8 @@ module.exports = {
 
   handler (argv) {
     argv.resolve((async () => {
-      const list = await argv.ipfs.bootstrap.rm(argv.peer, { all: argv.all })
+      const ipfs = await argv.getIpfs()
+      const list = await ipfs.bootstrap.rm(argv.peer, { all: argv.all })
       list.Peers.forEach((peer) => print(peer))
     })())
   }

src/cli/commands/cat.js

@@ -18,14 +18,18 @@ module.exports = {
     }
   },
 
-  handler ({ ipfs, ipfsPath, offset, length, resolve }) {
-    resolve(new Promise((resolve, reject) => {
-      const stream = ipfs.catReadableStream(ipfsPath, { offset, length })
+  handler ({ getIpfs, ipfsPath, offset, length, resolve }) {
+    resolve((async () => {
+      const ipfs = await getIpfs()
 
-      stream.on('error', reject)
-      stream.on('end', resolve)
+      return new Promise((resolve, reject) => {
+        const stream = ipfs.catReadableStream(ipfsPath, { offset, length })
 
-      stream.pipe(process.stdout)
-    }))
+        stream.on('error', reject)
+        stream.on('end', resolve)
+
+        stream.pipe(process.stdout)
+      })
+    })())
   }
 }

src/cli/commands/config.js

@@ -30,12 +30,13 @@ module.exports = {
       }
       argv._handled = true
 
-      const { bool, json, key } = argv
+      const { bool, json, key, getIpfs } = argv
+      const ipfs = await getIpfs()
       let value = argv.value
 
       if (!value) {
         // Get the value of a given key
-        value = await argv.ipfs.config.get(key)
+        value = await ipfs.config.get(key)
 
         if (typeof value === 'object') {
           print(JSON.stringify(value, null, 2))
@@ -55,7 +56,7 @@ module.exports = {
           }
         }
 
-        await argv.ipfs.config.set(key, value)
+        await ipfs.config.set(key, value)
       }
     })())
   }

src/cli/commands/config/edit.js

@@ -24,9 +24,11 @@ module.exports = {
         throw new Error('ENV variable $EDITOR not set')
       }
 
+      const ipfs = await argv.getIpfs()
+
       async function getConfig () {
         try {
-          await argv.ipfs.config.get()
+          await ipfs.config.get()
         } catch (err) {
           throw new Error('failed to get the config')
         }
@@ -76,7 +78,7 @@ module.exports = {
           ? Buffer.from(JSON.stringify(config)) : config
 
         try {
-          await argv.ipfs.config.replace(config)
+          await ipfs.config.replace(config)
         } catch (err) {
           throw new Error('failed to save the config')
         }