Skip to content

Commit

Permalink
insulate file resource from failures of file watching
Browse files Browse the repository at this point in the history 
Integration tests revealed that nsfw fails from time to time as well as introduce delays. It affects the file resource especially for the delete operation. Instead of relying only on nsfw, file resource now listens to file delete triggered by a user from Theia. It allows to react faster as well as tolerate failures of file watching.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
  • Loading branch information
@akosyakov
akosyakov committed Feb 18, 2020
1 parent 46a19a6 commit 00c1747
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 32 deletions.
10 changes: 10 additions & 0 deletions packages/filesystem/src/browser/file-resource.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ export class FileResource implements Resource {
this.sync();
}
}));
this.toDispose.push(this.fileSystemWatcher.onDidDelete(event => {
if (event.uri.isEqualOrParent(this.uri)) {
this.sync();
}
}));
this.toDispose.push(this.fileSystemWatcher.onDidMove(event => {
if (event.sourceUri.isEqualOrParent(this.uri) || event.targetUri.isEqualOrParent(this.uri)) {
this.sync();
}
}));
try {
this.toDispose.push(await this.fileSystemWatcher.watchFileChanges(this.uri));
} catch (e) {
Expand Down
80 changes: 51 additions & 29 deletions packages/filesystem/src/browser/filesystem-watcher.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { injectable, inject, postConstruct } from 'inversify';
import { Disposable, DisposableCollection } from '@theia/core/lib/common/disposable';
import { Emitter, Event, WaitUntilEvent } from '@theia/core/lib/common/event';
import { Emitter, WaitUntilEvent } from '@theia/core/lib/common/event';
import URI from '@theia/core/lib/common/uri';
import { FileSystem, FileShouldOverwrite } from '../common/filesystem';
import { DidFilesChangedParams, FileChangeType, FileSystemWatcherServer, WatchOptions } from '../common/filesystem-watcher-protocol';
Expand Down Expand Up @@ -77,7 +77,40 @@ export namespace FileMoveEvent {
}
}

export interface FileWillMoveEvent extends FileMoveEvent {
export interface FileEvent extends WaitUntilEvent {
uri: URI
}

export class FileOperationEmitter<E extends WaitUntilEvent> implements Disposable {

protected readonly onWillEmitter = new Emitter<E>();
readonly onWill = this.onWillEmitter.event;

protected readonly onDidFailEmitter = new Emitter<E>();
readonly onDidFail = this.onDidFailEmitter.event;

protected readonly onDidEmitter = new Emitter<E>();
readonly onDid = this.onDidEmitter.event;

protected readonly toDispose = new DisposableCollection(
this.onWillEmitter,
this.onDidFailEmitter,
this.onDidEmitter
);

dispose(): void {
this.toDispose.dispose();
}

async fireWill(event: Pick<E, Exclude<keyof E, 'waitUntil'>>): Promise<void> {
await WaitUntilEvent.fire(this.onWillEmitter, event);
}

async fireDid(failed: boolean, event: Pick<E, Exclude<keyof E, 'waitUntil'>>): Promise<void> {
const onDidEmitter = failed ? this.onDidFailEmitter : this.onDidEmitter;
await WaitUntilEvent.fire(onDidEmitter, event);
}

}

@injectable()
Expand All @@ -87,16 +120,17 @@ export class FileSystemWatcher implements Disposable {
protected readonly toRestartAll = new DisposableCollection();

protected readonly onFileChangedEmitter = new Emitter<FileChangeEvent>();
readonly onFilesChanged: Event<FileChangeEvent> = this.onFileChangedEmitter.event;
readonly onFilesChanged = this.onFileChangedEmitter.event;

protected readonly onWillMoveEmitter = new Emitter<FileWillMoveEvent>();
readonly onWillMove: Event<FileWillMoveEvent> = this.onWillMoveEmitter.event;
protected readonly fileDeleteEmitter = new FileOperationEmitter<FileEvent>();
readonly onWillDelete = this.fileDeleteEmitter.onWill;
readonly onDidFailDelete = this.fileDeleteEmitter.onDidFail;
readonly onDidDelete = this.fileDeleteEmitter.onDid;

protected readonly onDidFailMoveEmitter = new Emitter<FileMoveEvent>();
readonly onDidFailMove: Event<FileMoveEvent> = this.onDidFailMoveEmitter.event;

protected readonly onDidMoveEmitter = new Emitter<FileMoveEvent>();
readonly onDidMove: Event<FileMoveEvent> = this.onDidMoveEmitter.event;
protected readonly fileMoveEmitter = new FileOperationEmitter<FileMoveEvent>();
readonly onWillMove = this.fileMoveEmitter.onWill;
readonly onDidFailMove = this.fileMoveEmitter.onDidFail;
readonly onDidMove = this.fileMoveEmitter.onDid;

@inject(FileSystemWatcherServer)
protected readonly server: FileSystemWatcherServer;
Expand All @@ -115,8 +149,8 @@ export class FileSystemWatcher implements Disposable {
@postConstruct()
protected init(): void {
this.toDispose.push(this.onFileChangedEmitter);
this.toDispose.push(this.onDidFailMoveEmitter);
this.toDispose.push(this.onDidMoveEmitter);
this.toDispose.push(this.fileDeleteEmitter);
this.toDispose.push(this.fileMoveEmitter);

this.toDispose.push(this.server);
this.server.setClient({
Expand All @@ -131,8 +165,10 @@ export class FileSystemWatcher implements Disposable {

this.filesystem.setClient({
shouldOverwrite: this.shouldOverwrite.bind(this),
willMove: this.fireWillMove.bind(this),
didMove: this.fireDidMove.bind(this)
willDelete: uri => this.fileDeleteEmitter.fireWill({ uri: new URI(uri) }),
didDelete: (uri, failed) => this.fileDeleteEmitter.fireDid(failed, { uri: new URI(uri) }),
willMove: (source, target) => this.fileMoveEmitter.fireWill({ sourceUri: new URI(source), targetUri: new URI(target) }),
didMove: (source, target, failed) => this.fileMoveEmitter.fireDid(failed, { sourceUri: new URI(source), targetUri: new URI(target) })
});
}

Expand Down Expand Up @@ -191,19 +227,5 @@ export class FileSystemWatcher implements Disposable {
return Object.keys(patterns).filter(pattern => patterns[pattern]);
}

protected fireWillMove(sourceUri: string, targetUri: string): Promise<void> {
return WaitUntilEvent.fire(this.onWillMoveEmitter, {
sourceUri: new URI(sourceUri),
targetUri: new URI(targetUri)
});
}

protected fireDidMove(sourceUri: string, targetUri: string, failed: boolean): Promise<void> {
const onDidMoveEmitter = failed ? this.onDidFailMoveEmitter : this.onDidMoveEmitter;
return WaitUntilEvent.fire(onDidMoveEmitter, {
sourceUri: new URI(sourceUri),
targetUri: new URI(targetUri)
});
}

}

12 changes: 12 additions & 0 deletions packages/filesystem/src/common/filesystem.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ export interface FileSystemClient {
*/
shouldOverwrite: FileShouldOverwrite;

willDelete(uri: string): Promise<void>;

didDelete(uri: string, failed: boolean): Promise<void>;

willMove(sourceUri: string, targetUri: string): Promise<void>;

didMove(sourceUri: string, targetUri: string, failed: boolean): Promise<void>;
Expand All @@ -228,6 +232,14 @@ export class DispatchingFileSystemClient implements FileSystemClient {
);
}

async willDelete(uri: string): Promise<void> {
await Promise.all([...this.clients].map(client => client.willDelete(uri)));
}

async didDelete(uri: string, failed: boolean): Promise<void> {
await Promise.all([...this.clients].map(client => client.didDelete(uri, failed)));
}

async willMove(sourceUri: string, targetUri: string): Promise<void> {
await Promise.all([...this.clients].map(client => client.willMove(sourceUri, targetUri)));
}
Expand Down
25 changes: 24 additions & 1 deletion packages/filesystem/src/node/node-filesystem.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,23 @@ export class FileSystemNode implements FileSystem {
}

async delete(uri: string, options?: FileDeleteOptions): Promise<void> {
if (this.client) {
await this.client.willDelete(uri);
}
let failed = false;
try {
await this.doDelete(uri, options);
} catch (e) {
failed = true;
throw e;
} finally {
if (this.client) {
await this.client.didDelete(uri, failed);
}
}
}

protected async doDelete(uri: string, options?: FileDeleteOptions): Promise<void> {
const _uri = new URI(uri);
const stat = await this.doGetStat(_uri, 0);
if (!stat) {
Expand All @@ -335,7 +352,13 @@ export class FileSystemNode implements FileSystem {
const filePath = FileUri.fsPath(_uri);
const outputRootPath = paths.join(os.tmpdir(), v4());
try {
await fs.rename(filePath, outputRootPath);
await new Promise((resolve, reject) => mv(filePath, outputRootPath, { mkdirp: true, clobber: true }, async error => {
if (error) {
reject(error);
return;
}
resolve(undefined);
}));
// There is no reason for the promise returned by this function not to resolve
// as soon as the move is complete. Clearing up the temporary files can be
// done in the background.
Expand Down
4 changes: 2 additions & 2 deletions packages/plugin-ext/src/main/browser/in-plugin-filesystem-watcher-manager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { injectable, inject, postConstruct } from 'inversify';
import { parse, ParsedPattern, IRelativePattern } from '@theia/languages/lib/common/language-selector';
import { FileSystemWatcher, FileChangeEvent, FileChangeType, FileChange, FileMoveEvent, FileWillMoveEvent } from '@theia/filesystem/lib/browser/filesystem-watcher';
import { FileSystemWatcher, FileChangeEvent, FileChangeType, FileChange, FileMoveEvent } from '@theia/filesystem/lib/browser/filesystem-watcher';
import { WorkspaceExt } from '../../common/plugin-api-rpc';
import { FileWatcherSubscriberOptions } from '../../common/plugin-api-rpc-model';
import { RelativePattern } from '../../plugin/types-impl';
Expand Down Expand Up @@ -85,7 +85,7 @@ export class InPluginFileSystemWatcherManager {
}

// Filter file system changes according to subscribers settings here to avoid unneeded traffic.
protected onWillMoveEventHandler(change: FileWillMoveEvent): void {
protected onWillMoveEventHandler(change: FileMoveEvent): void {
for (const [id, subscriber] of this.subscribers) {
subscriber.proxy.$onWillRename({
subscriberId: id,
Expand Down

0 comments on commit 00c1747

Please sign in to comment.