Skip to content

Commit

Permalink
Fix FileResource sometimes sending contents change
Browse files Browse the repository at this point in the history 
event during writing

Add a lock to the write operation so that checking
if the file is synced must wait for the write
operation to be done.

Fixes eclipse-theia#14021

Contributed on behalf of Toro Cloud

Signed-off-by: Vivien Jovet <[email protected]>
  • Loading branch information
@vjovet-toro-cloud
vjovet-toro-cloud committed Aug 14, 2024
1 parent 817c1a0 commit c3c3456
Show file tree
Hide file tree
Showing 2 changed files with 267 additions and 0 deletions.
255 changes: 255 additions & 0 deletions packages/filesystem/src/browser/file-resource.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,255 @@
// *****************************************************************************
// Copyright (C) 2024 Toro Cloud Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// http://www.eclipse.org/legal/epl-2.0.
//
// This Source Code may also be made available under the following Secondary
// Licenses when the conditions for such availability set forth in the Eclipse
// Public License v. 2.0 are satisfied: GNU General Public License, version 2
// with the GNU Classpath Exception which is available at
// https://www.gnu.org/software/classpath/license.html.
//
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
// *****************************************************************************

import { enableJSDOM } from '@theia/core/lib/browser/test/jsdom';
let disableJSDOM = enableJSDOM();

import { FrontendApplicationConfigProvider } from '@theia/core/lib/browser/frontend-application-config-provider';
FrontendApplicationConfigProvider.set({});

import { Disposable, Emitter, URI } from '@theia/core';
import { Deferred } from '@theia/core/lib/common/promise-util';
import { expect } from 'chai';
import * as sinon from 'sinon';
import { FileChangesEvent, FileChangeType, FileStatWithMetadata } from '../common/files';
import { FileResource } from './file-resource';
import { FileService } from './file-service';

disableJSDOM();

describe.only('file-resource', () => {
const sandbox = sinon.createSandbox();
const mockEmitter = new Emitter();
const mockOnChangeEmitter = new Emitter<FileChangesEvent>();
const mockFileService = new FileService();

before(() => {
disableJSDOM = enableJSDOM();
});

beforeEach(() => {
sandbox.restore();

sandbox.stub(mockFileService, 'onDidFilesChange').get(() =>
mockOnChangeEmitter.event
);
sandbox.stub(mockFileService, 'onDidRunOperation').returns(Disposable.NULL);
sandbox.stub(mockFileService, 'watch').get(() =>
mockEmitter.event
);
sandbox.stub(mockFileService, 'onDidChangeFileSystemProviderCapabilities').get(() =>
mockEmitter.event
);
sandbox.stub(mockFileService, 'onDidChangeFileSystemProviderReadOnlyMessage').get(() =>
mockEmitter.event
);
});

after(() => {
disableJSDOM();
});

it('should save contents and not trigger change event', async () => {
const resource = new FileResource(new URI('file://test/file.txt'),
mockFileService, { readOnly: false, shouldOpenAsText: () => Promise.resolve(true), shouldOverwrite: () => Promise.resolve(true) });

const onChangeSpy = sandbox.spy();
resource.onDidChangeContents(onChangeSpy);

const deferred = new Deferred<FileStatWithMetadata & { encoding: string }>();

sandbox.stub(mockFileService, 'write')
.callsFake(() =>
deferred.promise
);

sandbox.stub(mockFileService, 'resolve')
.resolves({
mtime: 1,
ctime: 0,
size: 0,
etag: '',
isFile: true,
isDirectory: false,
isSymbolicLink: false,
isReadonly: false,
name: 'file.txt',
resource: new URI('file://test/file.txt')
});

resource.saveContents!('test');

await new Promise(resolve => setTimeout(resolve, 0));

mockOnChangeEmitter.fire(new FileChangesEvent(
[{
resource: new URI('file://test/file.txt'),
type: FileChangeType.UPDATED
}]
));

await new Promise(resolve => setImmediate(resolve));

expect(onChangeSpy.called).to.be.false;

deferred.resolve({
mtime: 0,
ctime: 0,
size: 0,
etag: '',
encoding: 'utf-8',
isFile: true,
isDirectory: false,
isSymbolicLink: false,
isReadonly: false,
name: 'file.txt',
resource: new URI('file://test/file.txt')
});

await new Promise(resolve => setImmediate(resolve));

expect(resource.version).to.deep.equal({ etag: '', mtime: 0, encoding: 'utf-8' });
});

it('should save content changes and not trigger change event', async () => {
sandbox.stub(mockFileService, 'hasCapability').returns(true);

const resource = new FileResource(new URI('file://test/file.txt'),
mockFileService, { readOnly: false, shouldOpenAsText: () => Promise.resolve(true), shouldOverwrite: () => Promise.resolve(true) });

const onChangeSpy = sandbox.spy();
resource.onDidChangeContents(onChangeSpy);

sandbox.stub(mockFileService, 'read')
.resolves({
mtime: 1,
ctime: 0,
size: 0,
etag: '',
name: 'file.txt',
resource: new URI('file://test/file.txt'),
value: 'test',
encoding: 'utf-8'
});

await resource.readContents!();

const deferred = new Deferred<FileStatWithMetadata & { encoding: string }>();

sandbox.stub(mockFileService, 'update')
.callsFake(() =>
deferred.promise
);

sandbox.stub(mockFileService, 'resolve')
.resolves({
mtime: 1,
ctime: 0,
size: 0,
etag: '',
isFile: true,
isDirectory: false,
isSymbolicLink: false,
isReadonly: false,
name: 'file.txt',
resource: new URI('file://test/file.txt')
});

resource.saveContentChanges!([{
range: { start: { line: 0, character: 0 }, end: { line: 0, character: 0 } },
rangeLength: 0,
text: 'test'
}]);

await new Promise(resolve => setTimeout(resolve, 0));

mockOnChangeEmitter.fire(new FileChangesEvent(
[{
resource: new URI('file://test/file.txt'),
type: FileChangeType.UPDATED
}]
));

await new Promise(resolve => setImmediate(resolve));

expect(onChangeSpy.called).to.be.false;

deferred.resolve({
mtime: 0,
ctime: 0,
size: 0,
etag: '',
encoding: 'utf-8',
isFile: true,
isDirectory: false,
isSymbolicLink: false,
isReadonly: false,
name: 'file.txt',
resource: new URI('file://test/file.txt')
});

await new Promise(resolve => setImmediate(resolve));

expect(resource.version).to.deep.equal({ etag: '', mtime: 0, encoding: 'utf-8' });
});

it('should trigger change event if file is updated and not in sync', async () => {
const resource = new FileResource(new URI('file://test/file.txt'),
mockFileService, { readOnly: false, shouldOpenAsText: () => Promise.resolve(true), shouldOverwrite: () => Promise.resolve(true) });

const onChangeSpy = sandbox.spy();
resource.onDidChangeContents(onChangeSpy);

sandbox.stub(mockFileService, 'read')
.resolves({
mtime: 1,
ctime: 0,
size: 0,
etag: '',
name: 'file.txt',
resource: new URI('file://test/file.txt'),
value: 'test',
encoding: 'utf-8'
});

await resource.readContents!();

sandbox.stub(mockFileService, 'resolve')
.resolves({
mtime: 2,
ctime: 0,
size: 0,
etag: '',
isFile: true,
isDirectory: false,
isSymbolicLink: false,
isReadonly: false,
name: 'file.txt',
resource: new URI('file://test/file.txt')
});

mockOnChangeEmitter.fire(new FileChangesEvent(
[{
resource: new URI('file://test/file.txt'),
type: FileChangeType.UPDATED
}]
));

await new Promise(resolve => setImmediate(resolve));

expect(onChangeSpy.called).to.be.true;
});
});
12 changes: 12 additions & 0 deletions packages/filesystem/src/browser/file-resource.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { GENERAL_MAX_FILE_SIZE_MB } from './filesystem-preferences';
import { FrontendApplicationStateService } from '@theia/core/lib/browser/frontend-application-state';
import { nls } from '@theia/core';
import { MarkdownString } from '@theia/core/lib/common/markdown-rendering';
import { Mutex } from 'async-mutex';

export interface FileResourceVersion extends ResourceVersion {
readonly encoding: string;
Expand Down Expand Up @@ -69,6 +70,8 @@ export class FileResource implements Resource {
return this.options.readOnly;
}

protected writingLock = new Mutex();

constructor(
readonly uri: URI,
protected readonly fileService: FileService,
Expand Down Expand Up @@ -216,6 +219,8 @@ export class FileResource implements Resource {
const version = options?.version || this._version;
const current = FileResourceVersion.is(version) ? version : undefined;
const etag = current?.etag;
const releaseLock = await this.writingLock.acquire();

try {
const stat = await this.fileService.write(this.uri, content, {
encoding: options?.encoding,
Expand All @@ -237,6 +242,8 @@ export class FileResource implements Resource {
throw ResourceError.OutOfSync({ message, stack, data: { uri: this.uri } });
}
throw e;
} finally {
releaseLock();
}
};

Expand All @@ -263,6 +270,8 @@ export class FileResource implements Resource {
throw ResourceError.NotFound({ message: 'has not been read yet', data: { uri: this.uri } });
}
const etag = current?.etag;
const releaseLock = await this.writingLock.acquire();

try {
const stat = await this.fileService.update(this.uri, changes, {
readEncoding: current.encoding,
Expand All @@ -286,6 +295,8 @@ export class FileResource implements Resource {
throw ResourceError.OutOfSync({ message, stack, data: { uri: this.uri } });
}
throw e;
} finally {
releaseLock();
}
};

Expand All @@ -303,6 +314,7 @@ export class FileResource implements Resource {
}
protected async isInSync(): Promise<boolean> {
try {
await this.writingLock.waitForUnlock();
const stat = await this.fileService.resolve(this.uri, { resolveMetadata: true });
return !!this.version && this.version.mtime >= stat.mtime;
} catch {
Expand Down

0 comments on commit c3c3456

Please sign in to comment.