Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WorkspaceEdit: Problem recreating a renamed resource #42634

Closed
aeschli opened this issue Jan 31, 2018 · 6 comments
Closed

WorkspaceEdit: Problem recreating a renamed resource #42634

aeschli opened this issue Jan 31, 2018 · 6 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workspace-edit
Milestone

Comments

@aeschli
Copy link
Contributor

aeschli commented Jan 31, 2018

Testing #42334

Run the extension below.

  • it does some edits to the document in the current editor
  • renames the document (adds a _ to the name)
  • creates a document with the original name
  • adds edits to the newly created document

-> you end up with an error dialog: Failed to save 'package.json': The content on disk is newer.
All edits are applied to the old resource name
image

'use strict';
import * as vscode from 'vscode';
import * as path from 'path';

export function activate(context: vscode.ExtensionContext) {
    let disposable = vscode.commands.registerCommand('extension.sayHello', () => {
        let editor = vscode.window.activeTextEditor;
        if (editor) {
            let doc = editor.document;
            let docUri = doc.uri;
            let newUri = nameWithUnderscore(docUri);

            let we = new vscode.WorkspaceEdit();
            we.insert(docUri, new vscode.Position(0, 0), 'Hello');
            we.insert(docUri, new vscode.Position(0, 0), 'Foo');
            we.renameResource(docUri, newUri);

            we.createResource(docUri);
            we.insert(docUri, new vscode.Position(0, 0), 'Bar');

            console.log(JSON.stringify(we.allEntries(), null, '\t'));
            

            vscode.workspace.applyEdit(we);
        }
    });

    context.subscriptions.push(disposable);
}

function nameWithUnderscore(uri: vscode.Uri) {
    let uriPath = uri.fsPath;
    return vscode.Uri.file(path.join(path.dirname(uriPath), '_' + path.basename(uriPath)));
}
@aeschli
Copy link
Contributor Author

aeschli commented Jan 31, 2018

The output shows that all edits were grouped together, although they belong to different resources

@aeschli
Copy link
Contributor Author

aeschli commented Jan 31, 2018

Same problem when recreating a deleted resource:

'use strict';
import * as vscode from 'vscode';
import * as path from 'path';

export function activate(context: vscode.ExtensionContext) {
    let disposable = vscode.commands.registerCommand('extension.sayHello', () => {
        let editor = vscode.window.activeTextEditor;
        if (editor) {
            let doc = editor.document;
            let docUri = doc.uri;

            let we = new vscode.WorkspaceEdit();
            we.insert(docUri, new vscode.Position(0, 0), 'Hello');
            we.insert(docUri, new vscode.Position(0, 0), 'Foo');
            we.deleteResource(docUri);

            we.createResource(docUri);
            we.insert(docUri, new vscode.Position(0, 0), 'Bar');

            console.log(JSON.stringify(we.allEntries(), null, '\t'));
            
            vscode.workspace.applyEdit(we);
        }
    });

    context.subscriptions.push(disposable);
}

@jrieken jrieken added the bug Issue identified by VS Code Team member as probable bug label Jan 31, 2018
@jrieken jrieken added this to the January 2018 milestone Jan 31, 2018
@aeschli aeschli changed the title workspaceEdit: Problem recreating a renamed resource WorkspaceEdit: Problem recreating a renamed resource Jan 31, 2018
@jrieken jrieken modified the milestones: January 2018, February 2018 Feb 1, 2018
@jrieken jrieken modified the milestones: February 2018, March 2018 Feb 27, 2018
@jrieken jrieken removed this from the March 2018 milestone Mar 8, 2018
@jrieken jrieken assigned bpasero and unassigned jrieken Jun 19, 2018
@jrieken jrieken added this to the June 2018 milestone Jun 19, 2018
@jrieken
Copy link
Member

jrieken commented Jun 19, 2018

@bpasero This doesn't seem to work. I have added this test https://github.com/Microsoft/vscode/blob/7631f97270bbb638e270898fa3efd754b2015483/extensions/vscode-api-tests/src/singlefolder-tests/workspace.test.ts#L592 and it fails because the newly created file has contents from the old file (the one that got renamed). To repro

  • enable the backup service
  • remove the comments

@bpasero
Copy link
Member

bpasero commented Jun 20, 2018

@jrieken the original issue from @aeschli no longer reproduces and the file is created just fine with the "bar" contents.

As for the test, I actually don't know how to fix it to run green without causing a flaky test. The issue is this:

Running Martins Code

  • the editor is opened (active editor) and being renamed
  • this causes the workbench to dispose the input of the editor which in turn disposes Alex' model eventually
  • the file is opened again
  • the contents change to "bar"
  • everything is fine

Running the Test

  • the model is actually not opened in any editor and is renamed
  • this means the old model is still hanging around because nobody is disposing it as nobody is listening to file changes
  • the file is opened again but since the model still exists, the contents are the same
  • "bar" is inserted
  • the test fails

I could in theory fix this by just destroying the model in ITextFileService#move() because I know that after the rename operation it is no longer valid, however this violates our model of model references and ref counting (the last reference that gets dispose should cause the model to get destroyed).

When I actually change the test to open the document in the editor first, the test succeeds 50% of the times, which is odd because it means there is a timing issue with reacting to the editor getting closed. If there was a way for the test to somehow synchronize on that event we could probably make it more robust, but for now I do not want to push a flaky test...

@jrieken
Copy link
Member

jrieken commented Jun 20, 2018

I could in theory fix this by just destroying the model in ITextFileService#move() because I know that after the rename operation it is no longer valid,

One less aggressive and more pragmatic approach would be to reset the model, e.g. set its value to '' and set the language mode to the null-language.

When I actually change the test to open the document in the editor first, the test succeeds 50% of the times, which is odd because it means there is a timing issue with reacting to the editor getting closed.

Hm, it'd expect 100% failure because vscode.workspace.openTextDocument holds a ref-counted reference which it releases after 3 minutes or when extensions have opened more than 80MB of documents (see BoundModelReferenceCollection). We could tweak that code to drop references for files that have been deleted/renamed

@bpasero
Copy link
Member

bpasero commented Jun 20, 2018

Let's continue that in #52414 so that we can let @aeschli verify that at least his scenario is working now.

@bpasero bpasero closed this as completed Jun 20, 2018
@aeschli aeschli added the verified Verification succeeded label Jun 28, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded workspace-edit
Projects
None yet
Development

No branches or pull requests

3 participants