From 5e25d58e2467a2fe2c6be6f6abf9409ded92e0ef Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 2 Jun 2021 11:11:14 -0700 Subject: [PATCH] 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. --- lib/cli.js | 2 +- lib/utils/error-handler.js | 4 ++- lib/utils/update-notifier.js | 34 +++++++++++++------- test/lib/cli.js | 1 + test/lib/utils/update-notifier.js | 53 +++++++++++++++++-------------- 5 files changed, 56 insertions(+), 38 deletions(-) diff --git a/lib/cli.js b/lib/cli.js index f42132f944390..d4a67645858ae 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 1fc31df44ffb9..da716679d2705 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 1af948a2db45a..ed5806ced2a7d 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 f491c6174b85e..42e05cc5d14c3 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 9748a92a8ae7b..dc0a64ff46127 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']) })