From 31dfab2f7c5f72b163cf2c012d15bbea5550779a Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Wed, 12 Jan 2022 11:32:41 +0100 Subject: [PATCH] files - add more tests and fix `promisify` issue with `fs.read` and `fs.write` --- src/vs/base/node/pfs.ts | 38 ++++++++++++++- src/vs/code/node/cli.ts | 2 +- src/vs/platform/files/common/watcher.ts | 48 +++++++++---------- .../files/node/diskFileSystemProvider.ts | 16 +------ .../node/watcher/nodejs/nodejsWatcher.ts | 5 +- .../node/nodejsWatcher.integrationTest.ts | 23 ++++++++- 6 files changed, 89 insertions(+), 43 deletions(-) diff --git a/src/vs/base/node/pfs.ts b/src/vs/base/node/pfs.ts index 7ea4c75c94b..a04892a4731 100644 --- a/src/vs/base/node/pfs.ts +++ b/src/vs/base/node/pfs.ts @@ -663,10 +663,44 @@ export const Promises = new class { get lstat() { return promisify(fs.lstat); } get utimes() { return promisify(fs.utimes); } - get read() { return promisify(fs.read); } + get read() { + + // Not using `promisify` here for a reason: the return + // type is not an object as indicated by TypeScript but + // just the bytes read, so we create our own wrapper. + + return (fd: number, buffer: Uint8Array, offset: number, length: number, position: number | null) => { + return new Promise<{ bytesRead: number, buffer: Uint8Array }>((resolve, reject) => { + fs.read(fd, buffer, offset, length, position, (err, bytesRead, buffer) => { + if (err) { + return reject(err); + } + + return resolve({ bytesRead, buffer }); + }); + }); + }; + } get readFile() { return promisify(fs.readFile); } - get write() { return promisify(fs.write); } + get write() { + + // Not using `promisify` here for a reason: the return + // type is not an object as indicated by TypeScript but + // just the bytes written, so we create our own wrapper. + + return (fd: number, buffer: Uint8Array, offset: number | undefined | null, length: number | undefined | null, position: number | undefined | null) => { + return new Promise<{ bytesWritten: number, buffer: Uint8Array }>((resolve, reject) => { + fs.write(fd, buffer, offset, length, position, (err, bytesWritten, buffer) => { + if (err) { + return reject(err); + } + + return resolve({ bytesWritten, buffer }); + }); + }); + }; + } get appendFile() { return promisify(fs.appendFile); } diff --git a/src/vs/code/node/cli.ts b/src/vs/code/node/cli.ts index a8be84cfce2..f564b6af707 100644 --- a/src/vs/code/node/cli.ts +++ b/src/vs/code/node/cli.ts @@ -402,7 +402,7 @@ export async function main(argv: string[]): Promise { const cts = new CancellationTokenSource(); child.on('close', () => cts.dispose(true)); - await watchFileContents(tmpName, chunk => stream.write(chunk), cts.token); + await watchFileContents(tmpName, chunk => stream.write(chunk), () => { /* ignore */ }, cts.token); } finally { unlinkSync(tmpName); } diff --git a/src/vs/platform/files/common/watcher.ts b/src/vs/platform/files/common/watcher.ts index fe27cac4faf..9bee90f45ab 100644 --- a/src/vs/platform/files/common/watcher.ts +++ b/src/vs/platform/files/common/watcher.ts @@ -9,6 +9,30 @@ import { isLinux } from 'vs/base/common/platform'; import { URI as uri } from 'vs/base/common/uri'; import { FileChangeType, IFileChange, isParent } from 'vs/platform/files/common/files'; +export interface IWatchRequest { + + /** + * The path to watch. + */ + path: string; + + /** + * A set of glob patterns or paths to exclude from watching. + */ + excludes: string[]; +} + +export interface INonRecursiveWatchRequest extends IWatchRequest { } + +export interface IRecursiveWatchRequest extends IWatchRequest { + + /** + * @deprecated this only exists for WSL1 support and should never + * be used in any other case. + */ + pollingInterval?: number; +} + export interface IRecursiveWatcher extends IDisposable { /** @@ -144,30 +168,6 @@ export abstract class AbstractRecursiveWatcherClient extends Disposable { } } -export interface IWatchRequest { - - /** - * The path to watch. - */ - path: string; - - /** - * A set of glob patterns or paths to exclude from watching. - */ - excludes: string[]; -} - -export interface INonRecursiveWatchRequest extends IWatchRequest { } - -export interface IRecursiveWatchRequest extends IWatchRequest { - - /** - * @deprecated this only exists for WSL1 support and should never - * be used in any other case. - */ - pollingInterval?: number; -} - export interface IDiskFileChange { type: FileChangeType; path: string; diff --git a/src/vs/platform/files/node/diskFileSystemProvider.ts b/src/vs/platform/files/node/diskFileSystemProvider.ts index d07067aea14..14d6a6a8559 100644 --- a/src/vs/platform/files/node/diskFileSystemProvider.ts +++ b/src/vs/platform/files/node/diskFileSystemProvider.ts @@ -410,13 +410,7 @@ export class DiskFileSystemProvider extends AbstractDiskFileSystemProvider imple let bytesRead: number | null = null; try { - const result = await Promises.read(fd, data, offset, length, normalizedPos); - - if (typeof result === 'number') { - bytesRead = result; // node.d.ts fail - } else { - bytesRead = result.bytesRead; - } + const { bytesRead } = await Promises.read(fd, data, offset, length, normalizedPos); return bytesRead; } catch (error) { @@ -496,13 +490,7 @@ export class DiskFileSystemProvider extends AbstractDiskFileSystemProvider imple let bytesWritten: number | null = null; try { - const result = await Promises.write(fd, data, offset, length, normalizedPos); - - if (typeof result === 'number') { - bytesWritten = result; // node.d.ts fail - } else { - bytesWritten = result.bytesWritten; - } + const { bytesWritten } = await Promises.write(fd, data, offset, length, normalizedPos); return bytesWritten; } catch (error) { diff --git a/src/vs/platform/files/node/watcher/nodejs/nodejsWatcher.ts b/src/vs/platform/files/node/watcher/nodejs/nodejsWatcher.ts index 1dd6b878f57..c51ce5e04cb 100644 --- a/src/vs/platform/files/node/watcher/nodejs/nodejsWatcher.ts +++ b/src/vs/platform/files/node/watcher/nodejs/nodejsWatcher.ts @@ -465,7 +465,7 @@ export class NodeJSFileWatcher extends Disposable implements INonRecursiveWatche * Watch the provided `path` for changes and return * the data in chunks of `Uint8Array` for further use. */ -export async function watchFileContents(path: string, onData: (chunk: Uint8Array) => void, token: CancellationToken, bufferSize = 512): Promise { +export async function watchFileContents(path: string, onData: (chunk: Uint8Array) => void, onReady: () => void, token: CancellationToken, bufferSize = 512): Promise { const handle = await Promises.open(path, 'r'); const buffer = Buffer.allocUnsafe(bufferSize); @@ -508,6 +508,9 @@ export async function watchFileContents(path: string, onData: (chunk: Uint8Array })(); }); + await watcher.ready; + onReady(); + return new Promise((resolve, reject) => { cts.token.onCancellationRequested(async () => { watcher.dispose(); diff --git a/src/vs/platform/files/test/node/nodejsWatcher.integrationTest.ts b/src/vs/platform/files/test/node/nodejsWatcher.integrationTest.ts index fa688171bc3..c205da6a4b0 100644 --- a/src/vs/platform/files/test/node/nodejsWatcher.integrationTest.ts +++ b/src/vs/platform/files/test/node/nodejsWatcher.integrationTest.ts @@ -10,11 +10,12 @@ import { Promises, RimRafMode } from 'vs/base/node/pfs'; import { flakySuite, getPathFromAmdModule, getRandomTestPath } from 'vs/base/test/node/testUtils'; import { FileChangeType } from 'vs/platform/files/common/files'; import { IDiskFileChange } from 'vs/platform/files/common/watcher'; -import { NodeJSFileWatcher } from 'vs/platform/files/node/watcher/nodejs/nodejsWatcher'; +import { NodeJSFileWatcher, watchFileContents } from 'vs/platform/files/node/watcher/nodejs/nodejsWatcher'; import { isLinux, isMacintosh, isWindows } from 'vs/base/common/platform'; import { getDriveLetter } from 'vs/base/common/extpath'; import { ltrim } from 'vs/base/common/strings'; import { DeferredPromise } from 'vs/base/common/async'; +import { CancellationTokenSource } from 'vs/base/common/cancellation'; // this suite has shown flaky runs in Azure pipelines where // tasks would just hang and timeout after a while (not in @@ -454,4 +455,24 @@ import { DeferredPromise } from 'vs/base/common/async'; // Ensure watcher is now disposed await watcher.whenDisposed; }); + + test('watchFileContents', async function () { + const watchedPath = join(testDir, 'lorem.txt'); + + const cts = new CancellationTokenSource(); + + const readyPromise = new DeferredPromise(); + const chunkPromise = new DeferredPromise(); + const watchPromise = watchFileContents(watchedPath, () => chunkPromise.complete(), () => readyPromise.complete(), cts.token); + + await readyPromise.p; + + Promises.writeFile(watchedPath, 'Hello World'); + + await chunkPromise.p; + + cts.cancel(); // this will resolve `watchPromise` + + return watchPromise; + }); });