Skip to content

Commit 480b465

Browse files
bpaserobpasero
authored
backups - guard reading operations with IO queue as well (microsoft#186736) (microsoft#186889)
1 parent ce708a9 commit 480b465

File tree

2 files changed

+92
-158
lines changed

2 files changed

+92
-158
lines changed

‎src/vs/workbench/services/workingCopy/common/workingCopyBackupService.ts

Lines changed: 92 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { basename, isEqual, joinPath } from 'vs/base/common/resources';
6+
import { joinPath } from 'vs/base/common/resources';
77
import { URI } from 'vs/base/common/uri';
88
import { coalesce } from 'vs/base/common/arrays';
99
import { equals, deepClone } from 'vs/base/common/objects';
@@ -111,14 +111,6 @@ export class WorkingCopyBackupsModel {
111111
this.cache.delete(resource);
112112
}
113113

114-
move(source: URI, target: URI): void {
115-
const entry = this.cache.get(source);
116-
if (entry) {
117-
this.cache.delete(source);
118-
this.cache.set(target, entry);
119-
}
120-
}
121-
122114
clear(): void {
123115
this.cache.clear();
124116
}
@@ -230,43 +222,15 @@ class WorkingCopyBackupServiceImpl extends Disposable implements IWorkingCopyBac
230222
// Create backup model
231223
this.model = await WorkingCopyBackupsModel.create(this.backupWorkspaceHome, this.fileService);
232224

233-
// Migrate hashes as needed. We used to hash with a MD5
234-
// sum of the path but switched to our own simpler hash
235-
// to avoid a node.js dependency. We still want to
236-
// support the older hash to prevent dataloss, so we:
237-
// - iterate over all backups
238-
// - detect if the file name length is 32 (MD5 length)
239-
// - read the backup's target file path
240-
// - rename the backup to the new hash
241-
// - update the backup in our model
242-
for (const backupResource of this.model.get()) {
243-
if (basename(backupResource).length !== 32) {
244-
continue; // not a MD5 hash, already uses new hash function
245-
}
246-
247-
try {
248-
const identifier = await this.resolveIdentifier(backupResource, this.model);
249-
if (!identifier) {
250-
this.logService.warn(`Backup: Unable to read target URI of backup ${backupResource} for migration to new hash.`);
251-
continue;
252-
}
253-
254-
const expectedBackupResource = this.toBackupResource(identifier);
255-
if (!isEqual(expectedBackupResource, backupResource)) {
256-
await this.fileService.move(backupResource, expectedBackupResource, true);
257-
this.model.move(backupResource, expectedBackupResource);
258-
}
259-
} catch (error) {
260-
this.logService.error(`Backup: Unable to migrate backup ${backupResource} to new hash.`);
261-
}
262-
}
263-
264225
return this.model;
265226
}
266227

267228
async hasBackups(): Promise<boolean> {
268229
const model = await this.ready;
269230

231+
// Ensure to await any pending backup operations
232+
await this.joinBackups();
233+
270234
return model.count() > 0;
271235
}
272236

@@ -409,48 +373,60 @@ class WorkingCopyBackupServiceImpl extends Disposable implements IWorkingCopyBac
409373
async getBackups(): Promise<IWorkingCopyIdentifier[]> {
410374
const model = await this.ready;
411375

376+
// Ensure to await any pending backup operations
377+
await this.joinBackups();
378+
412379
const backups = await Promise.all(model.get().map(backupResource => this.resolveIdentifier(backupResource, model)));
413380

414381
return coalesce(backups);
415382
}
416383

417384
private async resolveIdentifier(backupResource: URI, model: WorkingCopyBackupsModel): Promise<IWorkingCopyIdentifier | undefined> {
385+
let res: IWorkingCopyIdentifier | undefined = undefined;
418386

419-
// Read the entire backup preamble by reading up to
420-
// `PREAMBLE_MAX_LENGTH` in the backup file until
421-
// the `PREAMBLE_END_MARKER` is found
422-
const backupPreamble = await this.readToMatchingString(backupResource, WorkingCopyBackupServiceImpl.PREAMBLE_END_MARKER, WorkingCopyBackupServiceImpl.PREAMBLE_MAX_LENGTH);
423-
if (!backupPreamble) {
424-
return undefined;
425-
}
387+
await this.ioOperationQueues.queueFor(backupResource).queue(async () => {
388+
if (!model.has(backupResource)) {
389+
return; // require backup to be present
390+
}
426391

427-
// Figure out the offset in the preamble where meta
428-
// information possibly starts. This can be `-1` for
429-
// older backups without meta.
430-
const metaStartIndex = backupPreamble.indexOf(WorkingCopyBackupServiceImpl.PREAMBLE_META_SEPARATOR);
431-
432-
// Extract the preamble content for resource and meta
433-
let resourcePreamble: string;
434-
let metaPreamble: string | undefined;
435-
if (metaStartIndex > 0) {
436-
resourcePreamble = backupPreamble.substring(0, metaStartIndex);
437-
metaPreamble = backupPreamble.substr(metaStartIndex + 1);
438-
} else {
439-
resourcePreamble = backupPreamble;
440-
metaPreamble = undefined;
441-
}
392+
// Read the entire backup preamble by reading up to
393+
// `PREAMBLE_MAX_LENGTH` in the backup file until
394+
// the `PREAMBLE_END_MARKER` is found
395+
const backupPreamble = await this.readToMatchingString(backupResource, WorkingCopyBackupServiceImpl.PREAMBLE_END_MARKER, WorkingCopyBackupServiceImpl.PREAMBLE_MAX_LENGTH);
396+
if (!backupPreamble) {
397+
return;
398+
}
399+
400+
// Figure out the offset in the preamble where meta
401+
// information possibly starts. This can be `-1` for
402+
// older backups without meta.
403+
const metaStartIndex = backupPreamble.indexOf(WorkingCopyBackupServiceImpl.PREAMBLE_META_SEPARATOR);
404+
405+
// Extract the preamble content for resource and meta
406+
let resourcePreamble: string;
407+
let metaPreamble: string | undefined;
408+
if (metaStartIndex > 0) {
409+
resourcePreamble = backupPreamble.substring(0, metaStartIndex);
410+
metaPreamble = backupPreamble.substr(metaStartIndex + 1);
411+
} else {
412+
resourcePreamble = backupPreamble;
413+
metaPreamble = undefined;
414+
}
415+
416+
// Try to parse the meta preamble for figuring out
417+
// `typeId` and `meta` if defined.
418+
const { typeId, meta } = this.parsePreambleMeta(metaPreamble);
442419

443-
// Try to parse the meta preamble for figuring out
444-
// `typeId` and `meta` if defined.
445-
const { typeId, meta } = this.parsePreambleMeta(metaPreamble);
420+
// Update model entry with now resolved meta
421+
model.update(backupResource, meta);
446422

447-
// Update model entry with now resolved meta
448-
model.update(backupResource, meta);
423+
res = {
424+
typeId: typeId ?? NO_TYPE_ID,
425+
resource: URI.parse(resourcePreamble)
426+
};
427+
});
449428

450-
return {
451-
typeId: typeId ?? NO_TYPE_ID,
452-
resource: URI.parse(resourcePreamble)
453-
};
429+
return res;
454430
}
455431

456432
private async readToMatchingString(backupResource: URI, matchingString: string, maximumBytesToRead: number): Promise<string | undefined> {
@@ -469,50 +445,57 @@ class WorkingCopyBackupServiceImpl extends Disposable implements IWorkingCopyBac
469445
const backupResource = this.toBackupResource(identifier);
470446

471447
const model = await this.ready;
472-
if (!model.has(backupResource)) {
473-
return undefined; // require backup to be present
474-
}
475448

476-
// Load the backup content and peek into the first chunk
477-
// to be able to resolve the meta data
478-
const backupStream = await this.fileService.readFileStream(backupResource);
479-
const peekedBackupStream = await peekStream(backupStream.value, 1);
480-
const firstBackupChunk = VSBuffer.concat(peekedBackupStream.buffer);
481-
482-
// We have seen reports (e.g. https://github.com/microsoft/vscode/issues/78500) where
483-
// if VSCode goes down while writing the backup file, the file can turn empty because
484-
// it always first gets truncated and then written to. In this case, we will not find
485-
// the meta-end marker ('\n') and as such the backup can only be invalid. We bail out
486-
// here if that is the case.
487-
const preambleEndIndex = firstBackupChunk.buffer.indexOf(WorkingCopyBackupServiceImpl.PREAMBLE_END_MARKER_CHARCODE);
488-
if (preambleEndIndex === -1) {
489-
this.logService.trace(`Backup: Could not find meta end marker in ${backupResource}. The file is probably corrupt (filesize: ${backupStream.size}).`);
490-
491-
return undefined;
492-
}
449+
let res: IResolvedWorkingCopyBackup<T> | undefined = undefined;
493450

494-
const preambelRaw = firstBackupChunk.slice(0, preambleEndIndex).toString();
451+
await this.ioOperationQueues.queueFor(backupResource).queue(async () => {
452+
if (!model.has(backupResource)) {
453+
return; // require backup to be present
454+
}
495455

496-
// Extract meta data (if any)
497-
let meta: T | undefined;
498-
const metaStartIndex = preambelRaw.indexOf(WorkingCopyBackupServiceImpl.PREAMBLE_META_SEPARATOR);
499-
if (metaStartIndex !== -1) {
500-
meta = this.parsePreambleMeta(preambelRaw.substr(metaStartIndex + 1)).meta as T;
501-
}
456+
// Load the backup content and peek into the first chunk
457+
// to be able to resolve the meta data
458+
const backupStream = await this.fileService.readFileStream(backupResource);
459+
const peekedBackupStream = await peekStream(backupStream.value, 1);
460+
const firstBackupChunk = VSBuffer.concat(peekedBackupStream.buffer);
461+
462+
// We have seen reports (e.g. https://github.com/microsoft/vscode/issues/78500) where
463+
// if VSCode goes down while writing the backup file, the file can turn empty because
464+
// it always first gets truncated and then written to. In this case, we will not find
465+
// the meta-end marker ('\n') and as such the backup can only be invalid. We bail out
466+
// here if that is the case.
467+
const preambleEndIndex = firstBackupChunk.buffer.indexOf(WorkingCopyBackupServiceImpl.PREAMBLE_END_MARKER_CHARCODE);
468+
if (preambleEndIndex === -1) {
469+
this.logService.trace(`Backup: Could not find meta end marker in ${backupResource}. The file is probably corrupt (filesize: ${backupStream.size}).`);
470+
471+
return undefined;
472+
}
502473

503-
// Update model entry with now resolved meta
504-
model.update(backupResource, meta);
474+
const preambelRaw = firstBackupChunk.slice(0, preambleEndIndex).toString();
505475

506-
// Build a new stream without the preamble
507-
const firstBackupChunkWithoutPreamble = firstBackupChunk.slice(preambleEndIndex + 1);
508-
let value: VSBufferReadableStream;
509-
if (peekedBackupStream.ended) {
510-
value = bufferToStream(firstBackupChunkWithoutPreamble);
511-
} else {
512-
value = prefixedBufferStream(firstBackupChunkWithoutPreamble, peekedBackupStream.stream);
513-
}
476+
// Extract meta data (if any)
477+
let meta: T | undefined;
478+
const metaStartIndex = preambelRaw.indexOf(WorkingCopyBackupServiceImpl.PREAMBLE_META_SEPARATOR);
479+
if (metaStartIndex !== -1) {
480+
meta = this.parsePreambleMeta(preambelRaw.substr(metaStartIndex + 1)).meta as T;
481+
}
482+
483+
// Update model entry with now resolved meta
484+
model.update(backupResource, meta);
485+
486+
// Build a new stream without the preamble
487+
const firstBackupChunkWithoutPreamble = firstBackupChunk.slice(preambleEndIndex + 1);
488+
let value: VSBufferReadableStream;
489+
if (peekedBackupStream.ended) {
490+
value = bufferToStream(firstBackupChunkWithoutPreamble);
491+
} else {
492+
value = prefixedBufferStream(firstBackupChunkWithoutPreamble, peekedBackupStream.stream);
493+
}
494+
495+
res = { value, meta };
496+
});
514497

515-
return { value, meta };
498+
return res;
516499
}
517500

518501
private parsePreambleMeta<T extends IWorkingCopyBackupMeta>(preambleMetaRaw: string | undefined): { typeId: string | undefined; meta: T | undefined } {

‎src/vs/workbench/services/workingCopy/test/electron-sandbox/workingCopyBackupService.test.ts

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1212,11 +1212,6 @@ suite('WorkingCopyBackupService', () => {
12121212
model.update(resource4);
12131213
assert.strictEqual(model.has(resource4), true);
12141214
assert.strictEqual(model.has(resource4, undefined, { foo: 'nothing' }), false);
1215-
1216-
const resource5 = URI.file('test4.html');
1217-
model.move(resource4, resource5);
1218-
assert.strictEqual(model.has(resource4), false);
1219-
assert.strictEqual(model.has(resource5), true);
12201215
});
12211216

12221217
test('create', async () => {
@@ -1245,50 +1240,6 @@ suite('WorkingCopyBackupService', () => {
12451240
});
12461241
});
12471242

1248-
suite('Hash migration', () => {
1249-
1250-
test('works', async () => {
1251-
const fooBackupId = toUntypedWorkingCopyId(fooFile);
1252-
const untitledBackupId = toUntypedWorkingCopyId(untitledFile);
1253-
const customBackupId = toUntypedWorkingCopyId(customFile);
1254-
1255-
const fooBackupPath = joinPath(workspaceBackupPath, fooFile.scheme, hashIdentifier(fooBackupId));
1256-
const untitledBackupPath = joinPath(workspaceBackupPath, untitledFile.scheme, hashIdentifier(untitledBackupId));
1257-
const customFileBackupPath = joinPath(workspaceBackupPath, customFile.scheme, hashIdentifier(customBackupId));
1258-
1259-
// Prepare backups of the old MD5 hash format
1260-
await fileService.createFolder(joinPath(workspaceBackupPath, fooFile.scheme));
1261-
await fileService.createFolder(joinPath(workspaceBackupPath, untitledFile.scheme));
1262-
await fileService.createFolder(joinPath(workspaceBackupPath, customFile.scheme));
1263-
await fileService.writeFile(joinPath(workspaceBackupPath, fooFile.scheme, '8a8589a2f1c9444b89add38166f50229'), VSBuffer.fromString(`${fooFile.toString()}\ntest file`));
1264-
await fileService.writeFile(joinPath(workspaceBackupPath, untitledFile.scheme, '13264068d108c6901b3592ea654fcd57'), VSBuffer.fromString(`${untitledFile.toString()}\ntest untitled`));
1265-
await fileService.writeFile(joinPath(workspaceBackupPath, customFile.scheme, 'bf018572af7b38746b502893bd0adf6c'), VSBuffer.fromString(`${customFile.toString()}\ntest custom`));
1266-
1267-
service.reinitialize(workspaceBackupPath);
1268-
1269-
const backups = await service.getBackups();
1270-
assert.strictEqual(backups.length, 3);
1271-
assert.ok(backups.some(backup => isEqual(backup.resource, fooFile)));
1272-
assert.ok(backups.some(backup => isEqual(backup.resource, untitledFile)));
1273-
assert.ok(backups.some(backup => isEqual(backup.resource, customFile)));
1274-
1275-
assert.strictEqual((await fileService.resolve(joinPath(workspaceBackupPath, fooFile.scheme))).children?.length, 1);
1276-
assert.strictEqual((await fileService.exists(fooBackupPath)), true);
1277-
assert.strictEqual((await fileService.readFile(fooBackupPath)).value.toString(), `${fooFile.toString()}\ntest file`);
1278-
assert.ok(service.hasBackupSync(fooBackupId));
1279-
1280-
assert.strictEqual((await fileService.resolve(joinPath(workspaceBackupPath, untitledFile.scheme))).children?.length, 1);
1281-
assert.strictEqual((await fileService.exists(untitledBackupPath)), true);
1282-
assert.strictEqual((await fileService.readFile(untitledBackupPath)).value.toString(), `${untitledFile.toString()}\ntest untitled`);
1283-
assert.ok(service.hasBackupSync(untitledBackupId));
1284-
1285-
assert.strictEqual((await fileService.resolve(joinPath(workspaceBackupPath, customFile.scheme))).children?.length, 1);
1286-
assert.strictEqual((await fileService.exists(customFileBackupPath)), true);
1287-
assert.strictEqual((await fileService.readFile(customFileBackupPath)).value.toString(), `${customFile.toString()}\ntest custom`);
1288-
assert.ok(service.hasBackupSync(customBackupId));
1289-
});
1290-
});
1291-
12921243
suite('typeId migration', () => {
12931244

12941245
test('works (when meta is missing)', async () => {

0 commit comments

Comments
 (0)