Skip to content

Commit

Permalink
Debt: avoid promise.cancel in zip.ts (#56656)
Browse files Browse the repository at this point in the history 
* avoid promise.cancel

* use token properly
  • Loading branch information
@bpasero
bpasero committed Aug 22, 2018
1 parent e0a6826 commit a493688
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 58 deletions.
8 changes: 7 additions & 1 deletion src/vs/base/node/extfs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<boolean> {
export function mkdirp(path: string, mode?: number, token?: CancellationToken): TPromise<boolean> {
const mkdir = () => {
return nfcall(fs.mkdir, path, mode).then(null, (mkdirErr: NodeJS.ErrnoException) => {

Expand Down Expand Up @@ -160,6 +161,11 @@ export function mkdirp(path: string, mode?: number): TPromise<boolean> {
// 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') {
Expand Down
126 changes: 71 additions & 55 deletions src/vs/base/node/zip.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<void> {
function extractEntry(stream: Readable, fileName: string, mode: number, targetPath: string, options: IOptions, token: CancellationToken): TPromise<void> {
const dirName = path.dirname(fileName);
const targetDirName = path.join(targetPath, dirName);
if (targetDirName.indexOf(targetPath) !== 0) {
Expand All @@ -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<void> {
let isCanceled = false;
let last = TPromise.wrap<any>(null);
function extractZip(zipfile: ZipFile, targetPath: string, options: IOptions, logService: ILogService): CancelablePromise<void> {
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<ZipFile> {
Expand Down
20 changes: 18 additions & 2 deletions src/vs/base/test/node/extfs/extfs.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () => { };

Expand Down Expand Up @@ -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);
});
});
});

0 comments on commit a493688

Please sign in to comment.