From a493688be027a31337e313291522c2b5ac57c387 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Wed, 22 Aug 2018 07:24:27 +0200 Subject: [PATCH] Debt: avoid promise.cancel in zip.ts (#56656) * avoid promise.cancel * use token properly --- src/vs/base/node/extfs.ts | 8 +- src/vs/base/node/zip.ts | 126 ++++++++++++---------- src/vs/base/test/node/extfs/extfs.test.ts | 20 +++- 3 files changed, 96 insertions(+), 58 deletions(-) diff --git a/src/vs/base/node/extfs.ts b/src/vs/base/node/extfs.ts index b52c4e36f0d..3b0b3866f3c 100644 --- a/src/vs/base/node/extfs.ts +++ b/src/vs/base/node/extfs.ts @@ -15,6 +15,7 @@ import * as uuid from 'vs/base/common/uuid'; import { TPromise } from 'vs/base/common/winjs.base'; import { encode, encodeStream } from 'vs/base/node/encoding'; import * as flow from 'vs/base/node/flow'; +import { CancellationToken } from 'vs/base/common/cancellation'; const loop = flow.loop; @@ -129,7 +130,7 @@ function doCopyFile(source: string, target: string, mode: number, callback: (err reader.pipe(writer); } -export function mkdirp(path: string, mode?: number): TPromise { +export function mkdirp(path: string, mode?: number, token?: CancellationToken): TPromise { const mkdir = () => { return nfcall(fs.mkdir, path, mode).then(null, (mkdirErr: NodeJS.ErrnoException) => { @@ -160,6 +161,11 @@ export function mkdirp(path: string, mode?: number): TPromise { // recursively mkdir return mkdir().then(null, (err: NodeJS.ErrnoException) => { + // Respect cancellation + if (token && token.isCancellationRequested) { + return TPromise.as(false); + } + // ENOENT: a parent folder does not exist yet, continue // to create the parent folder and then try again. if (err.code === 'ENOENT') { diff --git a/src/vs/base/node/zip.ts b/src/vs/base/node/zip.ts index 4ef2b283769..70005628cbd 100644 --- a/src/vs/base/node/zip.ts +++ b/src/vs/base/node/zip.ts @@ -7,12 +7,14 @@ import * as nls from 'vs/nls'; import * as path from 'path'; import { createWriteStream, WriteStream } from 'fs'; import { Readable } from 'stream'; -import { nfcall, ninvoke, SimpleThrottler } from 'vs/base/common/async'; +import { nfcall, ninvoke, SimpleThrottler, createCancelablePromise, CancelablePromise } from 'vs/base/common/async'; import { mkdirp, rimraf } from 'vs/base/node/pfs'; import { TPromise } from 'vs/base/common/winjs.base'; import { open as _openZip, Entry, ZipFile } from 'yauzl'; import * as yazl from 'yazl'; import { ILogService } from 'vs/platform/log/common/log'; +import { CancellationToken } from 'vs/base/common/cancellation'; +import { once } from 'vs/base/common/event'; export interface IExtractOptions { overwrite?: boolean; @@ -70,7 +72,7 @@ function toExtractError(err: Error): ExtractError { return new ExtractError(type, err); } -function extractEntry(stream: Readable, fileName: string, mode: number, targetPath: string, options: IOptions): TPromise { +function extractEntry(stream: Readable, fileName: string, mode: number, targetPath: string, options: IOptions, token: CancellationToken): TPromise { const dirName = path.dirname(fileName); const targetDirName = path.join(targetPath, dirName); if (targetDirName.indexOf(targetPath) !== 0) { @@ -79,72 +81,86 @@ function extractEntry(stream: Readable, fileName: string, mode: number, targetPa const targetFileName = path.join(targetPath, fileName); let istream: WriteStream; - return mkdirp(targetDirName).then(() => new TPromise((c, e) => { + + once(token.onCancellationRequested)(() => { + if (istream) { + istream.close(); + } + }); + + return mkdirp(targetDirName, void 0, token).then(() => new TPromise((c, e) => { + if (token.isCancellationRequested) { + return; + } + istream = createWriteStream(targetFileName, { mode }); istream.once('close', () => c(null)); istream.once('error', e); stream.once('error', e); stream.pipe(istream); - }, () => { - if (istream) { - istream.close(); - } })); } -function extractZip(zipfile: ZipFile, targetPath: string, options: IOptions, logService: ILogService): TPromise { - let isCanceled = false; - let last = TPromise.wrap(null); +function extractZip(zipfile: ZipFile, targetPath: string, options: IOptions, logService: ILogService): CancelablePromise { + let last = createCancelablePromise(() => Promise.resolve(null)); let extractedEntriesCount = 0; - return new TPromise((c, e) => { - const throttler = new SimpleThrottler(); + return createCancelablePromise(token => { + + once(token.onCancellationRequested)(() => { + logService.debug(targetPath, 'Cancelled.'); + last.cancel(); + zipfile.close(); + }); + + return new TPromise((c, e) => { + const throttler = new SimpleThrottler(); + + const readNextEntry = (token: CancellationToken) => { + if (token.isCancellationRequested) { + return; + } + + extractedEntriesCount++; + zipfile.readEntry(); + }; - const readNextEntry = () => { - extractedEntriesCount++; + zipfile.once('error', e); + zipfile.once('close', () => last.then(() => { + if (token.isCancellationRequested || zipfile.entryCount === extractedEntriesCount) { + c(null); + } else { + e(new ExtractError('Incomplete', new Error(nls.localize('incompleteExtract', "Incomplete. Found {0} of {1} entries", extractedEntriesCount, zipfile.entryCount)))); + } + }, e)); zipfile.readEntry(); - }; - - zipfile.once('error', e); - zipfile.once('close', () => last.then(() => { - if (isCanceled || zipfile.entryCount === extractedEntriesCount) { - c(null); - } else { - e(new ExtractError('Incomplete', new Error(nls.localize('incompleteExtract', "Incomplete. Found {0} of {1} entries", extractedEntriesCount, zipfile.entryCount)))); - } - }, e)); - zipfile.readEntry(); - zipfile.on('entry', (entry: Entry) => { - - if (isCanceled) { - return; - } - - if (!options.sourcePathRegex.test(entry.fileName)) { - readNextEntry(); - return; - } - - const fileName = entry.fileName.replace(options.sourcePathRegex, ''); - - // directory file names end with '/' - if (/\/$/.test(fileName)) { - const targetFileName = path.join(targetPath, fileName); - last = mkdirp(targetFileName).then(() => readNextEntry()); - return; - } - - const stream = ninvoke(zipfile, zipfile.openReadStream, entry); - const mode = modeFromEntry(entry); - - last = throttler.queue(() => stream.then(stream => extractEntry(stream, fileName, mode, targetPath, options).then(() => readNextEntry()))); + zipfile.on('entry', (entry: Entry) => { + + if (token.isCancellationRequested) { + return; + } + + if (!options.sourcePathRegex.test(entry.fileName)) { + readNextEntry(token); + return; + } + + const fileName = entry.fileName.replace(options.sourcePathRegex, ''); + + // directory file names end with '/' + if (/\/$/.test(fileName)) { + const targetFileName = path.join(targetPath, fileName); + last = createCancelablePromise(token => mkdirp(targetFileName, void 0, token).then(() => readNextEntry(token))); + return; + } + + const stream = ninvoke(zipfile, zipfile.openReadStream, entry); + const mode = modeFromEntry(entry); + + last = createCancelablePromise(token => throttler.queue(() => stream.then(stream => extractEntry(stream, fileName, mode, targetPath, options, token).then(() => readNextEntry(token))))); + }); }); - }, () => { - logService.debug(targetPath, 'Cancelled.'); - isCanceled = true; - last.cancel(); - zipfile.close(); - }).then(null, err => TPromise.wrapError(toExtractError(err))); + }); } function openZip(zipFile: string, lazy: boolean = false): TPromise { diff --git a/src/vs/base/test/node/extfs/extfs.test.ts b/src/vs/base/test/node/extfs/extfs.test.ts index d1740085fd0..aa45c33cc84 100644 --- a/src/vs/base/test/node/extfs/extfs.test.ts +++ b/src/vs/base/test/node/extfs/extfs.test.ts @@ -15,8 +15,7 @@ import { isLinux, isWindows } from 'vs/base/common/platform'; import * as uuid from 'vs/base/common/uuid'; import * as extfs from 'vs/base/node/extfs'; import { getPathFromAmdModule } from 'vs/base/common/amd'; - - +import { CancellationTokenSource } from 'vs/base/common/cancellation'; const ignore = () => { }; @@ -564,4 +563,21 @@ suite('Extfs', () => { extfs.del(parentDir, os.tmpdir(), done, ignore); }); }); + + test('mkdirp cancellation', (done) => { + const id = uuid.generateUuid(); + const parentDir = path.join(os.tmpdir(), 'vsctests', id); + const newDir = path.join(parentDir, 'extfs', id); + + const source = new CancellationTokenSource(); + + const mkdirpPromise = extfs.mkdirp(newDir, 493, source.token); + source.cancel(); + + return mkdirpPromise.then(res => { + assert.equal(res, false); + + extfs.del(parentDir, os.tmpdir(), done, ignore); + }); + }); }); -- GitLab