files2 - avoid 2 fs.exists() when writing a file

This commit is contained in:
Benjamin Pasero 2019-04-12 11:49:02 +02:00
parent e6215b9fc9
commit efd5704d8d
4 changed files with 50 additions and 42 deletions

View file

@ -182,7 +182,7 @@ export function detectEncodingByBOMFromBuffer(buffer: Buffer | null, bytesRead:
* If no BOM is detected, null will be passed to callback.
*/
export function detectEncodingByBOM(file: string): Promise<string | null> {
return stream.readExactlyByFile(file, 3).then(({ buffer, bytesRead }) => detectEncodingByBOMFromBuffer(buffer, bytesRead));
return stream.readExactlyByFile(file, 3).then(({ buffer, bytesRead }) => detectEncodingByBOMFromBuffer(buffer, bytesRead), error => null);
}
const MINIMUM_THRESHOLD = 0.2;

View file

@ -11,6 +11,14 @@ import { Readable } from 'stream';
import { getPathFromAmdModule } from 'vs/base/common/amd';
suite('Encoding', () => {
test('detectBOM does not return error for non existing file', async () => {
const file = getPathFromAmdModule(require, './fixtures/not-exist.css');
const detectedEncoding = await encoding.detectEncodingByBOM(file);
assert.equal(detectedEncoding, null);
});
test('detectBOM UTF-8', async () => {
const file = getPathFromAmdModule(require, './fixtures/some_utf8.css');

View file

@ -317,36 +317,12 @@ export class FileService2 extends Disposable implements IFileService {
const provider = this.throwIfFileSystemIsReadonly(await this.withProvider(resource));
// validate write
const exists = await this.exists(resource);
if (exists) {
const stat = await provider.stat(resource);
// file cannot be directory
if ((stat.type & FileType.Directory) !== 0) {
throw new Error(localize('fileIsDirectoryError', "Expected file {0} is actually a directory", resource.toString()));
}
// Dirty write prevention: if the file on disk has been changed and does not match our expected
// mtime and etag, we bail out to prevent dirty writing.
//
// First, we check for a mtime that is in the future before we do more checks. The assumption is
// that only the mtime is an indicator for a file that has changd on disk.
//
// Second, if the mtime has advanced, we compare the size of the file on disk with our previous
// one using the etag() function. Relying only on the mtime check has prooven to produce false
// positives due to file system weirdness (especially around remote file systems). As such, the
// check for size is a weaker check because it can return a false negative if the file has changed
// but to the same length. This is a compromise we take to avoid having to produce checksums of
// the file content for comparison which would be much slower to compute.
if (options && typeof options.mtime === 'number' && typeof options.etag === 'string' && options.mtime < stat.mtime && options.etag !== etag(stat.size, options.mtime)) {
throw new FileOperationError(localize('fileModifiedError', "File Modified Since"), FileOperationResult.FILE_MODIFIED_SINCE, options);
}
}
const stat = await this.validateWriteFile(provider, resource, options);
try {
// mkdir recursively as needed
if (!exists) {
if (!stat) {
await this.mkdirp(provider, dirname(resource));
}
@ -371,6 +347,38 @@ export class FileService2 extends Disposable implements IFileService {
return this.resolve(resource, { resolveMetadata: true });
}
private async validateWriteFile(provider: IFileSystemProvider, resource: URI, options?: IWriteFileOptions): Promise<IStat | undefined> {
let stat: IStat | undefined = undefined;
try {
stat = await provider.stat(resource);
} catch (error) {
return undefined; // file might not exist
}
// file cannot be directory
if ((stat.type & FileType.Directory) !== 0) {
throw new Error(localize('fileIsDirectoryError', "Expected file {0} is actually a directory", resource.toString()));
}
// Dirty write prevention: if the file on disk has been changed and does not match our expected
// mtime and etag, we bail out to prevent dirty writing.
//
// First, we check for a mtime that is in the future before we do more checks. The assumption is
// that only the mtime is an indicator for a file that has changd on disk.
//
// Second, if the mtime has advanced, we compare the size of the file on disk with our previous
// one using the etag() function. Relying only on the mtime check has prooven to produce false
// positives due to file system weirdness (especially around remote file systems). As such, the
// check for size is a weaker check because it can return a false negative if the file has changed
// but to the same length. This is a compromise we take to avoid having to produce checksums of
// the file content for comparison which would be much slower to compute.
if (options && typeof options.mtime === 'number' && typeof options.etag === 'string' && options.mtime < stat.mtime && options.etag !== etag(stat.size, options.mtime)) {
throw new FileOperationError(localize('fileModifiedError', "File Modified Since"), FileOperationResult.FILE_MODIFIED_SINCE, options);
}
return stat;
}
resolveContent(resource: URI, options?: IResolveContentOptions): Promise<IContent> {
return this.joinOnLegacy.then(legacy => legacy.resolveContent(resource, options));
}

View file

@ -9,7 +9,7 @@ import { TextFileService } from 'vs/workbench/services/textfile/common/textFileS
import { ITextFileService } from 'vs/workbench/services/textfile/common/textfiles';
import { registerSingleton } from 'vs/platform/instantiation/common/extensions';
import { URI } from 'vs/base/common/uri';
import { ITextSnapshot, IWriteTextFileOptions, IFileStatWithMetadata, IResourceEncoding, IResolveContentOptions, IFileService, stringToSnapshot, ICreateFileOptions, FileOperationError, FileOperationResult } from 'vs/platform/files/common/files';
import { ITextSnapshot, IWriteTextFileOptions, IFileStatWithMetadata, IResourceEncoding, IResolveContentOptions, stringToSnapshot, ICreateFileOptions, FileOperationError, FileOperationResult } from 'vs/platform/files/common/files';
import { Schemas } from 'vs/base/common/network';
import { exists, stat, chmod, rimraf } from 'vs/base/node/pfs';
import { join, dirname } from 'vs/base/common/path';
@ -252,8 +252,7 @@ export class EncodingOracle extends Disposable {
constructor(
@ITextResourceConfigurationService private textResourceConfigurationService: ITextResourceConfigurationService,
@IEnvironmentService private environmentService: IEnvironmentService,
@IWorkspaceContextService private contextService: IWorkspaceContextService,
@IFileService private fileService: IFileService
@IWorkspaceContextService private contextService: IWorkspaceContextService
) {
super();
@ -293,18 +292,11 @@ export class EncodingOracle extends Disposable {
return { encoding, addBOM: true };
}
// Existing UTF-8 file: check for options regarding BOM
if (encoding === UTF8 && await this.fileService.exists(resource)) {
// if we are to overwrite the encoding, we do not preserve it if found
if (options && options.overwriteEncoding) {
return { encoding, addBOM: false };
}
// otherwise preserve it if found
if (resource.scheme === Schemas.file && await detectEncodingByBOM(resource.fsPath) === UTF8) {
return { encoding, addBOM: true };
}
// Ensure that we preserve an existing BOM if found for UTF8
// unless we are instructed to overwrite the encoding
const overwriteEncoding = options && options.overwriteEncoding;
if (!overwriteEncoding && encoding === UTF8 && resource.scheme === Schemas.file && await detectEncodingByBOM(resource.fsPath) === UTF8) {
return { encoding, addBOM: true };
}
return { encoding, addBOM: false };