Skip to content

Commit 8bd0521

Browse files
bpaserobpasero
authored
Save participants are re-entrant (fix microsoft#166352) (microsoft#166434)
1 parent 3fbbb5b commit 8bd0521

File tree

4 files changed

+115
-20
lines changed

4 files changed

+115
-20
lines changed

‎src/vs/workbench/services/textfile/common/textFileEditorModel.ts‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,9 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
8989

9090
private versionId = 0;
9191
private bufferSavedVersionId: number | undefined;
92+
9293
private ignoreDirtyOnModelContentChange = false;
94+
private ignoreSaveFromSaveParticipants = false;
9395

9496
private static readonly UNDO_REDO_SAVE_PARTICIPANTS_AUTO_SAVE_THROTTLE_THRESHOLD = 500;
9597
private lastModelContentChangeFromUndoRedo: number | undefined = undefined;
@@ -738,6 +740,15 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
738740
let versionId = this.versionId;
739741
this.trace(`doSave(${versionId}) - enter with versionId ${versionId}`);
740742

743+
// Return early if saved from within save participant to break recursion
744+
//
745+
// Scenario: a save participant triggers a save() on the model
746+
if (this.ignoreSaveFromSaveParticipants) {
747+
this.trace(`doSave(${versionId}) - exit - refusing to save() recursively from save participant`);
748+
749+
return;
750+
}
751+
741752
// Lookup any running pending save for this versionId and return it if found
742753
//
743754
// Scenario: user invoked the save action multiple times quickly for the same contents
@@ -820,7 +831,12 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
820831

821832
// Run save participants unless save was cancelled meanwhile
822833
if (!saveCancellation.token.isCancellationRequested) {
823-
await this.textFileService.files.runSaveParticipants(this, { reason: options.reason ?? SaveReason.EXPLICIT }, saveCancellation.token);
834+
this.ignoreSaveFromSaveParticipants = true;
835+
try {
836+
await this.textFileService.files.runSaveParticipants(this, { reason: options.reason ?? SaveReason.EXPLICIT }, saveCancellation.token);
837+
} finally {
838+
this.ignoreSaveFromSaveParticipants = false;
839+
}
824840
}
825841
} catch (error) {
826842
this.logService.error(`[text file model] runSaveParticipants(${versionId}) - resulted in an error: ${error.toString()}`, this.resource.toString());

‎src/vs/workbench/services/textfile/test/browser/textFileEditorModel.test.ts‎

Lines changed: 47 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -815,51 +815,81 @@ suite('Files - TextFileEditorModel', () => {
815815
participant.dispose();
816816
});
817817

818-
test('Save Participant, calling save from within is unsupported but does not explode (sync save)', async function () {
818+
test('Save Participant, calling save from within is unsupported but does not explode (sync save, no model change)', async function () {
819819
const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
820820

821-
await testSaveFromSaveParticipant(model, false);
821+
await testSaveFromSaveParticipant(model, false, false, false);
822822

823823
model.dispose();
824824
});
825825

826-
test('Save Participant, calling save from within is unsupported but does not explode (async save)', async function () {
826+
test('Save Participant, calling save from within is unsupported but does not explode (async save, no model change)', async function () {
827827
const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
828828

829-
await testSaveFromSaveParticipant(model, true);
829+
await testSaveFromSaveParticipant(model, true, false, false);
830830

831831
model.dispose();
832832
});
833833

834-
async function testSaveFromSaveParticipant(model: TextFileEditorModel, async: boolean): Promise<void> {
834+
test('Save Participant, calling save from within is unsupported but does not explode (sync save, model change)', async function () {
835+
const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
835836

836-
let breakLoop = false;
837+
await testSaveFromSaveParticipant(model, false, true, false);
837838

838-
const participant = accessor.textFileService.files.addSaveParticipant({
839-
participate: async model => {
840-
if (breakLoop) {
841-
return;
842-
}
839+
model.dispose();
840+
});
841+
842+
test('Save Participant, calling save from within is unsupported but does not explode (async save, model change)', async function () {
843+
const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
844+
845+
await testSaveFromSaveParticipant(model, true, true, false);
846+
847+
model.dispose();
848+
});
849+
850+
test('Save Participant, calling save from within is unsupported but does not explode (force)', async function () {
851+
const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, toResource.call(this, '/path/index_async.txt'), 'utf8', undefined);
852+
853+
await testSaveFromSaveParticipant(model, false, false, true);
843854

844-
breakLoop = true;
855+
model.dispose();
856+
});
845857

858+
async function testSaveFromSaveParticipant(model: TextFileEditorModel, async: boolean, modelChange: boolean, force: boolean): Promise<void> {
859+
860+
const disposable = accessor.textFileService.files.addSaveParticipant({
861+
participate: async () => {
846862
if (async) {
847863
await timeout(10);
848864
}
849-
const newSavePromise = model.save();
850865

851-
// assert that this is the same promise as the outer one
852-
assert.strictEqual(savePromise, newSavePromise);
866+
if (modelChange) {
867+
model.updateTextEditorModel(createTextBufferFactory('bar'));
868+
869+
const newSavePromise = model.save(force ? { force } : undefined);
870+
871+
// assert that this is not the same promise as the outer one
872+
assert.notStrictEqual(savePromise, newSavePromise);
873+
874+
await newSavePromise;
875+
} else {
876+
const newSavePromise = model.save(force ? { force } : undefined);
877+
878+
// assert that this is the same promise as the outer one
879+
assert.strictEqual(savePromise, newSavePromise);
880+
881+
await savePromise;
882+
}
853883
}
854884
});
855885

856886
await model.resolve();
857887
model.updateTextEditorModel(createTextBufferFactory('foo'));
858888

859-
const savePromise = model.save();
889+
const savePromise = model.save(force ? { force } : undefined);
860890
await savePromise;
861891

862-
participant.dispose();
892+
disposable.dispose();
863893
}
864894

865895
test('backup and restore (simple)', async function () {

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -781,6 +781,8 @@ export class StoredFileWorkingCopy<M extends IStoredFileWorkingCopyModel> extend
781781

782782
private readonly saveSequentializer = new TaskSequentializer();
783783

784+
private ignoreSaveFromSaveParticipants = false;
785+
784786
async save(options: IStoredFileWorkingCopySaveOptions = Object.create(null)): Promise<boolean> {
785787
if (!this.isResolved()) {
786788
return false;
@@ -817,6 +819,15 @@ export class StoredFileWorkingCopy<M extends IStoredFileWorkingCopyModel> extend
817819
let versionId = this.versionId;
818820
this.trace(`doSave(${versionId}) - enter with versionId ${versionId}`);
819821

822+
// Return early if saved from within save participant to break recursion
823+
//
824+
// Scenario: a save participant triggers a save() on the working copy
825+
if (this.ignoreSaveFromSaveParticipants) {
826+
this.trace(`doSave(${versionId}) - exit - refusing to save() recursively from save participant`);
827+
828+
return;
829+
}
830+
820831
// Lookup any running pending save for this versionId and return it if found
821832
//
822833
// Scenario: user invoked the save action multiple times quickly for the same contents
@@ -902,7 +913,12 @@ export class StoredFileWorkingCopy<M extends IStoredFileWorkingCopyModel> extend
902913

903914
// Run save participants unless save was cancelled meanwhile
904915
if (!saveCancellation.token.isCancellationRequested) {
905-
await this.workingCopyFileService.runSaveParticipants(this, { reason: options.reason ?? SaveReason.EXPLICIT }, saveCancellation.token);
916+
this.ignoreSaveFromSaveParticipants = true;
917+
try {
918+
await this.workingCopyFileService.runSaveParticipants(this, { reason: options.reason ?? SaveReason.EXPLICIT }, saveCancellation.token);
919+
} finally {
920+
this.ignoreSaveFromSaveParticipants = false;
921+
}
906922
}
907923
} catch (error) {
908924
this.logService.error(`[stored file working copy] runSaveParticipants(${versionId}) - resulted in an error: ${error.toString()}`, this.resource.toString(), this.typeId);

‎src/vs/workbench/services/workingCopy/test/browser/storedFileWorkingCopy.test.ts‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import { IInstantiationService } from 'vs/platform/instantiation/common/instanti
1515
import { basename } from 'vs/base/common/resources';
1616
import { FileChangesEvent, FileChangeType, FileOperationError, FileOperationResult, NotModifiedSinceFileOperationError } from 'vs/platform/files/common/files';
1717
import { SaveReason, SaveSourceRegistry } from 'vs/workbench/common/editor';
18-
import { Promises } from 'vs/base/common/async';
18+
import { Promises, timeout } from 'vs/base/common/async';
1919
import { consumeReadable, consumeStream, isReadableStream } from 'vs/base/common/stream';
2020
import { runWithFakedTimers } from 'vs/base/test/common/timeTravelScheduler';
2121

@@ -752,6 +752,39 @@ suite('StoredFileWorkingCopy', function () {
752752
assert.strictEqual(participationCounter, 1);
753753
});
754754

755+
test('Save Participant, calling save from within is unsupported but does not explode (sync save)', async function () {
756+
await workingCopy.resolve();
757+
758+
await testSaveFromSaveParticipant(workingCopy, false);
759+
});
760+
761+
test('Save Participant, calling save from within is unsupported but does not explode (async save)', async function () {
762+
await workingCopy.resolve();
763+
764+
await testSaveFromSaveParticipant(workingCopy, true);
765+
});
766+
767+
async function testSaveFromSaveParticipant(workingCopy: StoredFileWorkingCopy<TestStoredFileWorkingCopyModel>, async: boolean): Promise<void> {
768+
769+
assert.strictEqual(accessor.workingCopyFileService.hasSaveParticipants, false);
770+
771+
const disposable = accessor.workingCopyFileService.addSaveParticipant({
772+
participate: async () => {
773+
if (async) {
774+
await timeout(10);
775+
}
776+
777+
await workingCopy.save({ force: true });
778+
}
779+
});
780+
781+
assert.strictEqual(accessor.workingCopyFileService.hasSaveParticipants, true);
782+
783+
await workingCopy.save({ force: true });
784+
785+
disposable.dispose();
786+
}
787+
755788
test('revert', async () => {
756789
await workingCopy.resolve();
757790
workingCopy.model?.updateContents('hello revert');

0 commit comments

Comments
 (0)