Skip to content

Commit b65a897

Browse files
WardenGnawWardenGnaw
authored
Adding file validation for download and unzip. (#1751)
* Adding file validation for download and unzip. Refactored the download retry code. Added error handlers for read and write streams. WriteStream writes to a *.tmp file and then renames. This catches the case if VSCode dies in the middle of this process and the file is partially written. We will continue where we left off. * Adding return to resolve/reject calls. * Fix retry loop and check err in unlink and rename * Actually wait for those promises * Fix TSLint error
1 parent 638fd67 commit b65a897

File tree

3 files changed

+126
-73
lines changed

3 files changed

+126
-73
lines changed

‎Extension/package-lock.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Extension/src/common.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,4 +408,26 @@ export function checkDistro(platformInfo: PlatformInformation): void {
408408
// or SunOS (the other platforms supported by node)
409409
getOutputChannelLogger().appendLine(`Warning: Debugging has not been tested for this platform. ${getReadmeMessage()}`);
410410
}
411+
}
412+
413+
export async function unlinkPromise(fileName: string): Promise<void> {
414+
return new Promise<void>((resolve, reject) => {
415+
fs.unlink(fileName, err => {
416+
if (err) {
417+
return reject(err);
418+
}
419+
return resolve();
420+
});
421+
});
422+
}
423+
424+
export async function renamePromise(oldName: string, newName: string): Promise<void> {
425+
return new Promise<void>((resolve, reject) => {
426+
fs.rename(oldName, newName, err => {
427+
if (err) {
428+
return reject(err);
429+
}
430+
return resolve();
431+
});
432+
});
411433
}

‎Extension/src/packageManager.ts

Lines changed: 103 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -137,69 +137,74 @@ export class PackageManager {
137137
});
138138
}
139139

140-
private DownloadPackage(pkg: IPackage): Promise<void> {
140+
private async DownloadPackage(pkg: IPackage): Promise<void> {
141141
this.AppendChannel(`Downloading package '${pkg.description}' `);
142142

143143
this.SetStatusText("$(cloud-download) Downloading packages...");
144144
this.SetStatusTooltip(`Downloading package '${pkg.description}'...`);
145145

146+
const tmpResult: tmp.SyncResult = await this.CreateTempFile(pkg);
147+
await this.DownloadPackageWithRetries(pkg, tmpResult);
148+
}
149+
150+
private async CreateTempFile(pkg: IPackage): Promise<tmp.SyncResult> {
146151
return new Promise<tmp.SyncResult>((resolve, reject) => {
147152
tmp.file({ prefix: "package-" }, (err, path, fd, cleanupCallback) => {
148153
if (err) {
149154
return reject(new PackageManagerError('Error from temp.file', 'DownloadPackage', pkg, err));
150155
}
151156

152-
resolve(<tmp.SyncResult>{ name: path, fd: fd, removeCallback: cleanupCallback });
157+
return resolve(<tmp.SyncResult>{ name: path, fd: fd, removeCallback: cleanupCallback });
153158
});
154-
})
155-
.then((tmpResult) => {
156-
pkg.tmpFile = tmpResult;
157-
158-
let lastError: any = null;
159-
let retryCount: number = 0;
160-
let handleDownloadFailure: (num: any, error: any) => void = (num, error) => {
161-
retryCount = num;
162-
lastError = error;
159+
});
160+
}
161+
162+
private async DownloadPackageWithRetries(pkg: IPackage, tmpResult: tmp.SyncResult): Promise<void> {
163+
pkg.tmpFile = tmpResult;
164+
165+
let success: boolean = false;
166+
let lastError: any = null;
167+
let retryCount: number = 0;
168+
const MAX_RETRIES: number = 5;
169+
170+
// Retry the download at most MAX_RETRIES times with 2-32 seconds delay.
171+
do {
172+
try {
173+
await this.DownloadFile(pkg.url, pkg, retryCount);
174+
success = true;
175+
} catch (error) {
176+
retryCount += 1;
177+
lastError = error;
178+
if (retryCount >= MAX_RETRIES) {
179+
this.AppendChannel(` Failed to download ` + pkg.url);
180+
throw error;
181+
} else {
182+
// This will skip the success = true.
163183
this.AppendChannel(` Failed. Retrying...`);
164-
};
165-
// Retry the download at most 5 times with 2-32 seconds delay.
166-
return this.DownloadFile(pkg.url, pkg, 0).catch((error) => {
167-
handleDownloadFailure(1, error);
168-
return this.DownloadFile(pkg.url, pkg, 1).catch((error) => {
169-
handleDownloadFailure(2, error);
170-
return this.DownloadFile(pkg.url, pkg, 2).catch((error) => {
171-
handleDownloadFailure(3, error);
172-
return this.DownloadFile(pkg.url, pkg, 3).catch((error) => {
173-
handleDownloadFailure(4, error);
174-
return this.DownloadFile(pkg.url, pkg, 4).catch((error) => {
175-
handleDownloadFailure(5, error);
176-
return this.DownloadFile(pkg.url, pkg, 5); // Last try, don't catch the error.
177-
});
178-
});
179-
});
180-
});
181-
}).then(() => {
182-
this.AppendLineChannel(" Done!");
183-
if (retryCount !== 0) {
184-
// Log telemetry to see if retrying helps.
185-
let telemetryProperties: { [key: string]: string } = {};
186-
telemetryProperties["success"] = `OnRetry${retryCount}`;
187-
if (lastError instanceof PackageManagerError) {
188-
let packageError: PackageManagerError = lastError;
189-
telemetryProperties['error.methodName'] = packageError.methodName;
190-
telemetryProperties['error.message'] = packageError.message;
191-
if (packageError.pkg) {
192-
telemetryProperties['error.packageName'] = packageError.pkg.description;
193-
telemetryProperties['error.packageUrl'] = packageError.pkg.url;
194-
}
195-
if (packageError.errorCode) {
196-
telemetryProperties['error.errorCode'] = packageError.errorCode;
197-
}
198-
}
199-
Telemetry.logDebuggerEvent("acquisition", telemetryProperties);
200-
}
201-
});
202-
});
184+
continue;
185+
}
186+
}
187+
} while (!success && retryCount < MAX_RETRIES);
188+
189+
this.AppendLineChannel(" Done!");
190+
if (retryCount !== 0) {
191+
// Log telemetry to see if retrying helps.
192+
let telemetryProperties: { [key: string]: string } = {};
193+
telemetryProperties["success"] = `OnRetry${retryCount}`;
194+
if (lastError instanceof PackageManagerError) {
195+
let packageError: PackageManagerError = lastError;
196+
telemetryProperties['error.methodName'] = packageError.methodName;
197+
telemetryProperties['error.message'] = packageError.message;
198+
if (packageError.pkg) {
199+
telemetryProperties['error.packageName'] = packageError.pkg.description;
200+
telemetryProperties['error.packageUrl'] = packageError.pkg.url;
201+
}
202+
if (packageError.errorCode) {
203+
telemetryProperties['error.errorCode'] = packageError.errorCode;
204+
}
205+
}
206+
Telemetry.logDebuggerEvent("acquisition", telemetryProperties);
207+
}
203208
}
204209

205210
// reloadCpptoolsJson in main.ts uses ~25% of this function.
@@ -276,11 +281,11 @@ export class PackageManager {
276281
});
277282

278283
response.on('end', () => {
279-
resolve();
284+
return resolve();
280285
});
281286

282287
response.on('error', (error) => {
283-
reject(new PackageManagerWebResponseError(response.socket, 'HTTP/HTTPS Response error', 'DownloadFile', pkg, error.stack, error.name));
288+
return reject(new PackageManagerWebResponseError(response.socket, 'HTTP/HTTPS Response error', 'DownloadFile', pkg, error.stack, error.name));
284289
});
285290

286291
// Begin piping data from the response to the package file
@@ -291,7 +296,7 @@ export class PackageManager {
291296
let request: ClientRequest = https.request(options, handleHttpResponse);
292297

293298
request.on('error', (error) => {
294-
reject(new PackageManagerError('HTTP/HTTPS Request error' + (urlString.includes("fwlink") ? ": fwlink" : ""), 'DownloadFile', pkg, error.stack, error.message));
299+
return reject(new PackageManagerError('HTTP/HTTPS Request error' + (urlString.includes("fwlink") ? ": fwlink" : ""), 'DownloadFile', pkg, error.stack, error.message));
295300
});
296301

297302
// Execute the request
@@ -316,6 +321,15 @@ export class PackageManager {
316321
return reject(new PackageManagerError('Zip file error', 'InstallPackage', pkg, err));
317322
}
318323

324+
// setup zip file events
325+
zipfile.on('end', () => {
326+
return resolve();
327+
});
328+
329+
zipfile.on('error', err => {
330+
return reject(new PackageManagerError('Zip File Error', 'InstallPackage', pkg, err, err.code));
331+
});
332+
319333
zipfile.readEntry();
320334

321335
zipfile.on('entry', (entry: yauzl.Entry) => {
@@ -334,26 +348,52 @@ export class PackageManager {
334348
util.checkFileExists(absoluteEntryPath).then((exists: boolean) => {
335349
if (!exists) {
336350
// File - extract it
337-
zipfile.openReadStream(entry, (err, readStream) => {
351+
zipfile.openReadStream(entry, (err, readStream: fs.ReadStream) => {
338352
if (err) {
339353
return reject(new PackageManagerError('Error reading zip stream', 'InstallPackage', pkg, err));
340354
}
341355

342-
mkdirp.mkdirp(path.dirname(absoluteEntryPath), { mode: 0o775 }, (err) => {
356+
readStream.on('error', (err) => {
357+
return reject(new PackageManagerError('Error in readStream', 'InstallPackage', pkg, err));
358+
});
359+
360+
mkdirp.mkdirp(path.dirname(absoluteEntryPath), { mode: 0o775 }, async (err) => {
343361
if (err) {
344362
return reject(new PackageManagerError('Error creating directory', 'InstallPackage', pkg, err, err.code));
345363
}
346364

365+
// Create as a .tmp file to avoid partially unzipped files
366+
// counting as completed files.
367+
let absoluteEntryTempFile: string = absoluteEntryPath + ".tmp";
368+
if (fs.existsSync(absoluteEntryTempFile)) {
369+
try {
370+
await util.unlinkPromise(absoluteEntryTempFile);
371+
} catch (err) {
372+
return reject(new PackageManagerError(`Error unlinking file ${absoluteEntryTempFile}`, 'InstallPackage', pkg, err));
373+
}
374+
}
375+
347376
// Make sure executable files have correct permissions when extracted
348377
let fileMode: number = (pkg.binaries && pkg.binaries.indexOf(absoluteEntryPath) !== -1) ? 0o755 : 0o664;
349-
350-
let writeStream: fs.WriteStream = fs.createWriteStream(absoluteEntryPath, { mode: fileMode });
351-
readStream.pipe(writeStream);
352-
writeStream.on('close', () => {
378+
let writeStream: fs.WriteStream = fs.createWriteStream(absoluteEntryTempFile, { mode: fileMode });
379+
380+
writeStream.on('close', async () => {
381+
try {
382+
// Remove .tmp extension from the file.
383+
await util.renamePromise(absoluteEntryTempFile, absoluteEntryPath);
384+
} catch (err) {
385+
return reject(new PackageManagerError(`Error renaming file ${absoluteEntryTempFile}`, 'InstallPackage', pkg, err));
386+
}
353387
// Wait till output is done writing before reading the next zip entry.
354388
// Otherwise, it's possible to try to launch the .exe before it is done being created.
355389
zipfile.readEntry();
356390
});
391+
392+
writeStream.on('error', (err) => {
393+
return reject(new PackageManagerError('Error in writeStream', 'InstallPackage', pkg, err));
394+
});
395+
396+
readStream.pipe(writeStream);
357397
});
358398
});
359399
} else {
@@ -366,20 +406,11 @@ export class PackageManager {
366406
});
367407
}
368408
});
369-
370-
zipfile.on('end', () => {
371-
resolve();
372-
});
373-
374-
zipfile.on('error', err => {
375-
reject(new PackageManagerError('Zip File Error', 'InstallPackage', pkg, err, err.code));
376-
});
377-
});
378-
})
379-
.then(() => {
380-
// Clean up temp file
381-
pkg.tmpFile.removeCallback();
382409
});
410+
}).then(() => {
411+
// Clean up temp file
412+
pkg.tmpFile.removeCallback();
413+
});
383414
}
384415

385416
private AppendChannel(text: string): void {

0 commit comments

Comments
 (0)