Skip to content

Commit

Permalink
fix(update-notifier): parallelize check for updates
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@isaacs
isaacs committed Jun 3, 2021
1 parent bc9c57d commit 5e25d58
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 38 deletions.
2 changes: 1 addition & 1 deletion lib/cli.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
4 changes: 3 additions & 1 deletion lib/utils/error-handler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
34 changes: 22 additions & 12 deletions lib/utils/update-notifier.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
Expand All @@ -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
Expand Down Expand Up @@ -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
}
1 change: 1 addition & 0 deletions test/lib/cli.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
53 changes: 29 additions & 24 deletions test/lib/utils/update-notifier.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,22 @@ 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)
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
})

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',
Expand All @@ -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',
Expand All @@ -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')
})
Expand All @@ -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')
})

Expand All @@ -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'])
})

Expand Down

0 comments on commit 5e25d58

Please sign in to comment.