fix(update-notifier): parallelize check for updates

This prevents npm from hanging for an update notification if the server
is slow to respond or the cache file is slow to write.

Failure mode is just that we notify more often than needed, which is not
so bad.

PR-URL: https://github.com/npm/cli/pull/3348
Credit: @isaacs
Close: #3348
Reviewed-by: @wraithgar

5 files changed, 56 insertions, 38 deletions

diff --git a/lib/cli.js b/lib/cli.js
index f42132f94..d4a676458 100644
--- a/lib/cli.js
+++ b/lib/cli.js
@@ -53,7 +53,7 @@ module.exports = (process) => {
       npm.config.set('usage', false, 'cli')
     }
 
-    npm.updateNotification = await updateNotifier(npm)
+    updateNotifier(npm)
 
     const cmd = npm.argv.shift()
     const impl = npm.commands[cmd]
diff --git a/lib/utils/error-handler.js b/lib/utils/error-handler.js
index 1fc31df44..da716679d 100644
--- a/lib/utils/error-handler.js
+++ b/lib/utils/error-handler.js
@@ -119,7 +119,9 @@ const errorHandler = (er) => {
   if (cbCalled)
     er = er || new Error('Callback called more than once.')
 
-  if (npm.updateNotification) {
+  // only show the notification if it finished before the other stuff we
+  // were doing.  no need to hang on `npm -v` or something.
+  if (typeof npm.updateNotification === 'string') {
     const { level } = log
     log.level = log.levels.notice
     log.notice('', npm.updateNotification)
diff --git a/lib/utils/update-notifier.js b/lib/utils/update-notifier.js
index 1af948a2d..ed5806ced 100644
--- a/lib/utils/update-notifier.js
+++ b/lib/utils/update-notifier.js
@@ -21,23 +21,25 @@ const isGlobalNpmUpdate = npm => {
 const DAILY = 1000 * 60 * 60 * 24
 const WEEKLY = DAILY * 7
 
-const updateTimeout = async (npm, duration) => {
+// don't put it in the _cacache folder, just in npm's cache
+const lastCheckedFile = npm =>
+  resolve(npm.flatOptions.cache, '../_update-notifier-last-checked')
+
+const checkTimeout = async (npm, duration) => {
   const t = new Date(Date.now() - duration)
-  // don't put it in the _cacache folder, just in npm's cache
-  const f = resolve(npm.flatOptions.cache, '../_update-notifier-last-checked')
+  const f = lastCheckedFile(npm)
   // if we don't have a file, then definitely check it.
   const st = await stat(f).catch(() => ({ mtime: t - 1 }))
+  return t > st.mtime
+}
 
-  if (t > st.mtime) {
-    // best effort, if this fails, it's ok.
-    // might be using /dev/null as the cache or something weird like that.
-    await writeFile(f, '').catch(() => {})
-    return true
-  } else
-    return false
+const updateTimeout = async npm => {
+  // best effort, if this fails, it's ok.
+  // might be using /dev/null as the cache or something weird like that.
+  await writeFile(lastCheckedFile(npm), '').catch(() => {})
 }
 
-const updateNotifier = module.exports = async (npm, spec = 'latest') => {
+const updateNotifier = async (npm, spec = 'latest') => {
   // never check for updates in CI, when updating npm already, or opted out
   if (!npm.config.get('update-notifier') ||
       isGlobalNpmUpdate(npm) ||
@@ -57,7 +59,7 @@ const updateNotifier = module.exports = async (npm, spec = 'latest') => {
   const duration = spec !== 'latest' ? DAILY : WEEKLY
 
   // if we've already checked within the specified duration, don't check again
-  if (!(await updateTimeout(npm, duration)))
+  if (!(await checkTimeout(npm, duration)))
     return null
 
   // if they're currently using a prerelease, nudge to the next prerelease
@@ -113,3 +115,11 @@ const updateNotifier = module.exports = async (npm, spec = 'latest') => {
 
   return messagec
 }
+
+// only update the notification timeout if we actually finished checking
+module.exports = async npm => {
+  const notification = await updateNotifier(npm)
+  // intentional.  do not await this.  it's a best-effort update.
+  updateTimeout(npm)
+  npm.updateNotification = notification
+}
diff --git a/test/lib/cli.js b/test/lib/cli.js
index f491c6174..42e05cc5d 100644
--- a/test/lib/cli.js
+++ b/test/lib/cli.js
@@ -45,6 +45,7 @@ const npmlogMock = {
 
 const cli = t.mock('../../lib/cli.js', {
   '../../lib/npm.js': npmock,
+  '../../lib/utils/update-notifier.js': async () => null,
   '../../lib/utils/did-you-mean.js': () => '\ntest did you mean',
   '../../lib/utils/unsupported.js': unsupportedMock,
   '../../lib/utils/error-handler.js': errorHandlerMock,
diff --git a/test/lib/utils/update-notifier.js b/test/lib/utils/update-notifier.js
index 9748a92a8..dc0a64ff4 100644
--- a/test/lib/utils/update-notifier.js
+++ b/test/lib/utils/update-notifier.js
@@ -86,9 +86,14 @@ t.afterEach(() => {
   WRITE_ERROR = null
 })
 
+const runUpdateNotifier = async npm => {
+  await updateNotifier(npm)
+  return npm.updateNotification
+}
+
 t.test('situations in which we do not notify', t => {
   t.test('nothing to do if notifier disabled', async t => {
-    t.equal(await updateNotifier({
+    t.equal(await runUpdateNotifier({
       ...npm,
       config: { get: (k) => k !== 'update-notifier' },
     }), null)
@@ -96,7 +101,7 @@ t.test('situations in which we do not notify', t => {
   })
 
   t.test('do not suggest update if already updating', async t => {
-    t.equal(await updateNotifier({
+    t.equal(await runUpdateNotifier({
       ...npm,
       flatOptions: { ...flatOptions, global: true },
       command: 'install',
@@ -106,7 +111,7 @@ t.test('situations in which we do not notify', t => {
   })
 
   t.test('do not suggest update if already updating with spec', async t => {
-    t.equal(await updateNotifier({
+    t.equal(await runUpdateNotifier({
       ...npm,
       flatOptions: { ...flatOptions, global: true },
       command: 'install',
@@ -116,31 +121,31 @@ t.test('situations in which we do not notify', t => {
   })
 
   t.test('do not update if same as latest', async t => {
-    t.equal(await updateNotifier(npm), null)
+    t.equal(await runUpdateNotifier(npm), null)
     t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('check if stat errors (here for coverage)', async t => {
     STAT_ERROR = new Error('blorg')
-    t.equal(await updateNotifier(npm), null)
+    t.equal(await runUpdateNotifier(npm), null)
     t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('ok if write errors (here for coverage)', async t => {
     WRITE_ERROR = new Error('grolb')
-    t.equal(await updateNotifier(npm), null)
+    t.equal(await runUpdateNotifier(npm), null)
     t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('ignore pacote failures (here for coverage)', async t => {
     PACOTE_ERROR = new Error('pah-KO-tchay')
-    t.equal(await updateNotifier(npm), null)
+    t.equal(await runUpdateNotifier(npm), null)
     t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
   })
   t.test('do not update if newer than latest, but same as next', async t => {
-    t.equal(await updateNotifier({ ...npm, version: NEXT_VERSION }), null)
+    t.equal(await runUpdateNotifier({ ...npm, version: NEXT_VERSION }), null)
     const reqs = ['npm@latest', `npm@^${NEXT_VERSION}`]
     t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
   })
   t.test('do not update if on the latest beta', async t => {
-    t.equal(await updateNotifier({ ...npm, version: CURRENT_BETA }), null)
+    t.equal(await runUpdateNotifier({ ...npm, version: CURRENT_BETA }), null)
     const reqs = [`npm@^${CURRENT_BETA}`]
     t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
   })
@@ -150,21 +155,21 @@ t.test('situations in which we do not notify', t => {
       ciMock = null
     })
     ciMock = 'something'
-    t.equal(await updateNotifier(npm), null)
+    t.equal(await runUpdateNotifier(npm), null)
     t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.test('only check weekly for GA releases', async t => {
     // One week (plus five minutes to account for test environment fuzziness)
     STAT_MTIME = Date.now() - (1000 * 60 * 60 * 24 * 7) + (1000 * 60 * 5)
-    t.equal(await updateNotifier(npm), null)
+    t.equal(await runUpdateNotifier(npm), null)
     t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
   t.test('only check daily for betas', async t => {
     // One day (plus five minutes to account for test environment fuzziness)
     STAT_MTIME = Date.now() - (1000 * 60 * 60 * 24) + (1000 * 60 * 5)
-    t.equal(await updateNotifier({ ...npm, version: HAVE_BETA }), null)
+    t.equal(await runUpdateNotifier({ ...npm, version: HAVE_BETA }), null)
     t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
   })
 
@@ -174,43 +179,43 @@ t.test('situations in which we do not notify', t => {
 t.test('notification situations', t => {
   t.test('new beta available', async t => {
     const version = HAVE_BETA
-    t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
-    t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
     t.strictSame(MANIFEST_REQUEST, [`npm@^${version}`, `npm@^${version}`])
   })
 
   t.test('patch to next version', async t => {
     const version = NEXT_PATCH
-    t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
-    t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
     t.strictSame(MANIFEST_REQUEST, ['npm@latest', `npm@^${version}`, 'npm@latest', `npm@^${version}`])
   })
 
   t.test('minor to next version', async t => {
     const version = NEXT_MINOR
-    t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
-    t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
     t.strictSame(MANIFEST_REQUEST, ['npm@latest', `npm@^${version}`, 'npm@latest', `npm@^${version}`])
   })
 
   t.test('patch to current', async t => {
     const version = CURRENT_PATCH
-    t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
-    t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
     t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest'])
   })
 
   t.test('minor to current', async t => {
     const version = CURRENT_MINOR
-    t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
-    t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
     t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest'])
   })
 
   t.test('major to current', async t => {
     const version = CURRENT_MAJOR
-    t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
-    t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
+    t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
     t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest'])
   })