Skip to content

Commit aafe235

Browse files
isaacsisaacs
committed
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: #3348 Credit: @isaacs Close: #3348 Reviewed-by: @wraithgar
1 parent bc9c57d commit aafe235

File tree

5 files changed

+56
-38
lines changed

5 files changed

+56
-38
lines changed

‎lib/cli.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ module.exports = (process) => {
5353
npm.config.set('usage', false, 'cli')
5454
}
5555

56-
npm.updateNotification = await updateNotifier(npm)
56+
updateNotifier(npm)
5757

5858
const cmd = npm.argv.shift()
5959
const impl = npm.commands[cmd]

‎lib/utils/error-handler.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,9 @@ const errorHandler = (er) => {
119119
if (cbCalled)
120120
er = er || new Error('Callback called more than once.')
121121

122-
if (npm.updateNotification) {
122+
// only show the notification if it finished before the other stuff we
123+
// were doing. no need to hang on `npm -v` or something.
124+
if (typeof npm.updateNotification === 'string') {
123125
const { level } = log
124126
log.level = log.levels.notice
125127
log.notice('', npm.updateNotification)

‎lib/utils/update-notifier.js

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,23 +21,25 @@ const isGlobalNpmUpdate = npm => {
2121
const DAILY = 1000 * 60 * 60 * 24
2222
const WEEKLY = DAILY * 7
2323

24-
const updateTimeout = async (npm, duration) => {
24+
// don't put it in the _cacache folder, just in npm's cache
25+
const lastCheckedFile = npm =>
26+
resolve(npm.flatOptions.cache, '../_update-notifier-last-checked')
27+
28+
const checkTimeout = async (npm, duration) => {
2529
const t = new Date(Date.now() - duration)
26-
// don't put it in the _cacache folder, just in npm's cache
27-
const f = resolve(npm.flatOptions.cache, '../_update-notifier-last-checked')
30+
const f = lastCheckedFile(npm)
2831
// if we don't have a file, then definitely check it.
2932
const st = await stat(f).catch(() => ({ mtime: t - 1 }))
33+
return t > st.mtime
34+
}
3035

31-
if (t > st.mtime) {
32-
// best effort, if this fails, it's ok.
33-
// might be using /dev/null as the cache or something weird like that.
34-
await writeFile(f, '').catch(() => {})
35-
return true
36-
} else
37-
return false
36+
const updateTimeout = async npm => {
37+
// best effort, if this fails, it's ok.
38+
// might be using /dev/null as the cache or something weird like that.
39+
await writeFile(lastCheckedFile(npm), '').catch(() => {})
3840
}
3941

40-
const updateNotifier = module.exports = async (npm, spec = 'latest') => {
42+
const updateNotifier = async (npm, spec = 'latest') => {
4143
// never check for updates in CI, when updating npm already, or opted out
4244
if (!npm.config.get('update-notifier') ||
4345
isGlobalNpmUpdate(npm) ||
@@ -57,7 +59,7 @@ const updateNotifier = module.exports = async (npm, spec = 'latest') => {
5759
const duration = spec !== 'latest' ? DAILY : WEEKLY
5860

5961
// if we've already checked within the specified duration, don't check again
60-
if (!(await updateTimeout(npm, duration)))
62+
if (!(await checkTimeout(npm, duration)))
6163
return null
6264

6365
// if they're currently using a prerelease, nudge to the next prerelease
@@ -113,3 +115,11 @@ const updateNotifier = module.exports = async (npm, spec = 'latest') => {
113115

114116
return messagec
115117
}
118+
119+
// only update the notification timeout if we actually finished checking
120+
module.exports = async npm => {
121+
const notification = await updateNotifier(npm)
122+
// intentional. do not await this. it's a best-effort update.
123+
updateTimeout(npm)
124+
npm.updateNotification = notification
125+
}

‎test/lib/cli.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ const npmlogMock = {
4545

4646
const cli = t.mock('../../lib/cli.js', {
4747
'../../lib/npm.js': npmock,
48+
'../../lib/utils/update-notifier.js': async () => null,
4849
'../../lib/utils/did-you-mean.js': () => '\ntest did you mean',
4950
'../../lib/utils/unsupported.js': unsupportedMock,
5051
'../../lib/utils/error-handler.js': errorHandlerMock,

‎test/lib/utils/update-notifier.js

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -86,17 +86,22 @@ t.afterEach(() => {
8686
WRITE_ERROR = null
8787
})
8888

89+
const runUpdateNotifier = async npm => {
90+
await updateNotifier(npm)
91+
return npm.updateNotification
92+
}
93+
8994
t.test('situations in which we do not notify', t => {
9095
t.test('nothing to do if notifier disabled', async t => {
91-
t.equal(await updateNotifier({
96+
t.equal(await runUpdateNotifier({
9297
...npm,
9398
config: { get: (k) => k !== 'update-notifier' },
9499
}), null)
95100
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
96101
})
97102

98103
t.test('do not suggest update if already updating', async t => {
99-
t.equal(await updateNotifier({
104+
t.equal(await runUpdateNotifier({
100105
...npm,
101106
flatOptions: { ...flatOptions, global: true },
102107
command: 'install',
@@ -106,7 +111,7 @@ t.test('situations in which we do not notify', t => {
106111
})
107112

108113
t.test('do not suggest update if already updating with spec', async t => {
109-
t.equal(await updateNotifier({
114+
t.equal(await runUpdateNotifier({
110115
...npm,
111116
flatOptions: { ...flatOptions, global: true },
112117
command: 'install',
@@ -116,31 +121,31 @@ t.test('situations in which we do not notify', t => {
116121
})
117122

118123
t.test('do not update if same as latest', async t => {
119-
t.equal(await updateNotifier(npm), null)
124+
t.equal(await runUpdateNotifier(npm), null)
120125
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
121126
})
122127
t.test('check if stat errors (here for coverage)', async t => {
123128
STAT_ERROR = new Error('blorg')
124-
t.equal(await updateNotifier(npm), null)
129+
t.equal(await runUpdateNotifier(npm), null)
125130
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
126131
})
127132
t.test('ok if write errors (here for coverage)', async t => {
128133
WRITE_ERROR = new Error('grolb')
129-
t.equal(await updateNotifier(npm), null)
134+
t.equal(await runUpdateNotifier(npm), null)
130135
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
131136
})
132137
t.test('ignore pacote failures (here for coverage)', async t => {
133138
PACOTE_ERROR = new Error('pah-KO-tchay')
134-
t.equal(await updateNotifier(npm), null)
139+
t.equal(await runUpdateNotifier(npm), null)
135140
t.strictSame(MANIFEST_REQUEST, ['npm@latest'], 'requested latest version')
136141
})
137142
t.test('do not update if newer than latest, but same as next', async t => {
138-
t.equal(await updateNotifier({ ...npm, version: NEXT_VERSION }), null)
143+
t.equal(await runUpdateNotifier({ ...npm, version: NEXT_VERSION }), null)
139144
const reqs = ['npm@latest', `npm@^${NEXT_VERSION}`]
140145
t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
141146
})
142147
t.test('do not update if on the latest beta', async t => {
143-
t.equal(await updateNotifier({ ...npm, version: CURRENT_BETA }), null)
148+
t.equal(await runUpdateNotifier({ ...npm, version: CURRENT_BETA }), null)
144149
const reqs = [`npm@^${CURRENT_BETA}`]
145150
t.strictSame(MANIFEST_REQUEST, reqs, 'requested latest and next versions')
146151
})
@@ -150,21 +155,21 @@ t.test('situations in which we do not notify', t => {
150155
ciMock = null
151156
})
152157
ciMock = 'something'
153-
t.equal(await updateNotifier(npm), null)
158+
t.equal(await runUpdateNotifier(npm), null)
154159
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
155160
})
156161

157162
t.test('only check weekly for GA releases', async t => {
158163
// One week (plus five minutes to account for test environment fuzziness)
159164
STAT_MTIME = Date.now() - (1000 * 60 * 60 * 24 * 7) + (1000 * 60 * 5)
160-
t.equal(await updateNotifier(npm), null)
165+
t.equal(await runUpdateNotifier(npm), null)
161166
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
162167
})
163168

164169
t.test('only check daily for betas', async t => {
165170
// One day (plus five minutes to account for test environment fuzziness)
166171
STAT_MTIME = Date.now() - (1000 * 60 * 60 * 24) + (1000 * 60 * 5)
167-
t.equal(await updateNotifier({ ...npm, version: HAVE_BETA }), null)
172+
t.equal(await runUpdateNotifier({ ...npm, version: HAVE_BETA }), null)
168173
t.strictSame(MANIFEST_REQUEST, [], 'no requests for manifests')
169174
})
170175

@@ -174,43 +179,43 @@ t.test('situations in which we do not notify', t => {
174179
t.test('notification situations', t => {
175180
t.test('new beta available', async t => {
176181
const version = HAVE_BETA
177-
t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
178-
t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
182+
t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
183+
t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
179184
t.strictSame(MANIFEST_REQUEST, [`npm@^${version}`, `npm@^${version}`])
180185
})
181186

182187
t.test('patch to next version', async t => {
183188
const version = NEXT_PATCH
184-
t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
185-
t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
189+
t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
190+
t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
186191
t.strictSame(MANIFEST_REQUEST, ['npm@latest', `npm@^${version}`, 'npm@latest', `npm@^${version}`])
187192
})
188193

189194
t.test('minor to next version', async t => {
190195
const version = NEXT_MINOR
191-
t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
192-
t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
196+
t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
197+
t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
193198
t.strictSame(MANIFEST_REQUEST, ['npm@latest', `npm@^${version}`, 'npm@latest', `npm@^${version}`])
194199
})
195200

196201
t.test('patch to current', async t => {
197202
const version = CURRENT_PATCH
198-
t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
199-
t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
203+
t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
204+
t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
200205
t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest'])
201206
})
202207

203208
t.test('minor to current', async t => {
204209
const version = CURRENT_MINOR
205-
t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
206-
t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
210+
t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
211+
t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
207212
t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest'])
208213
})
209214

210215
t.test('major to current', async t => {
211216
const version = CURRENT_MAJOR
212-
t.matchSnapshot(await updateNotifier({ ...npm, version }), 'color')
213-
t.matchSnapshot(await updateNotifier({ ...npmNoColor, version }), 'no color')
217+
t.matchSnapshot(await runUpdateNotifier({ ...npm, version }), 'color')
218+
t.matchSnapshot(await runUpdateNotifier({ ...npmNoColor, version }), 'no color')
214219
t.strictSame(MANIFEST_REQUEST, ['npm@latest', 'npm@latest'])
215220
})
216221

0 commit comments

Comments
 (0)