microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.64k stars 28.67k forks source link

Different behaviors when applying workspace edit #119943

Closed jdneo closed 3 years ago

jdneo commented 3 years ago

Version: 1.54.3 (system setup) Commit: 2b9aebd5354a3629c3aba0a5f5df49f43d6689f8 Date: 2021-03-15T10:55:45.459Z Electron: 11.3.0 Chrome: 87.0.4280.141 Node.js: 12.18.3 V8: 8.7.220.31-electron.0 OS: Windows_NT x64 10.0.19042

Steps to Reproduce:

  1. Clone the extension for test purpose: https://github.com/jdneo/vscode-workspace-edit-issue

    This extension registers onDidCreateFiles() event listener and will insert a snippet when a new file is created. Meanwhile, it registers a command which will apply a workspace edit to create a new file and insert some words into the file. See: https://github.com/jdneo/vscode-workspace-edit-issue/blob/master/src/extension.ts#L6-L19

  2. After setup the extension, debug it.
  3. Open an empty folder in VS Code
  4. Trigger the command Hello Wrold, a file generated contains all the content from the snippet and workspace edit.
  5. Remove the generated file
  6. Trigger the command Hello Wrold again, the content from the workspace edit is not applied.

https://user-images.githubusercontent.com/6193897/112606383-49694e80-8e53-11eb-8338-845ea0821ead.mp4

Does this issue occur when all extensions are disabled?: Yes/No

jrieken commented 3 years ago

Seeing this error

ERR Error: file:///Users/jrieken/Code/_samples/devfest/NewFile has changed in the meantime
    at BulkTextEdits._validateBeforePrepare (bulkTextEdits.js:132)
    at BulkTextEdits.apply (bulkTextEdits.js:186)
    at BulkEdit._performTextEdits (bulkEditService.js:91)
    at BulkEdit.perform (bulkEditService.js:72)
    at async BulkEditService.apply (bulkEditService.js:165)
jrieken commented 3 years ago

Tho, this only happens when you don't save the file before deleting it

jdneo commented 3 years ago

@jrieken I'm curious what's the expected behavior for this situation when multiple edits are going to apply to the same document. Who should be the one to win the race condition 😄

I mean, I do not find some guidance in the document about this situation. How should the extension authors deal with this?

jdneo commented 3 years ago

@jrieken I can repro this both saving/without saving the new file before deleting it.

Just add some more background about this issue:

In VS Code Java, we registered a listener for the new file event to add file headers into that new file via snippet.

Meanwhile, there are some other code actions which will generate new files via workspace edit.

Today the behavior is: The workspace edit will take effect for the first time. But if the generated file is deleted and trigger the code action again, the workspace edit will not take effect anymore.

jrieken commented 3 years ago

This is quite interesting and deep. @jdneo for now I might only have a workaround for you...

Trouble starts when delete happens. We don't send an onDidClose-event to the extension host because there are still references to that document (one is from the openTextDocument-call). This means the extension still has the text document active and at a certain version. That by itself isn't a problem. Now, when the command is run a second time the following happens:

  1. we create the file and pick-up the existing text document (at version 3)
  2. we apply the initial text edit (the "Created from workspace edit" string) which increases the version to 4
  3. the onDidCreate-event is fired
  4. the extension is told about the new version of the text document

Now, at step 3 the "snippet-edit" is prepared and that's for version 3 - which is now outdated and therefore dropped...

jrieken commented 3 years ago

I am looking into a fix for this but it might be more complex than I anticipated. As a workaround the following should work: When handling the onDidCreateFiles-event debounce things by a little (e.g. setTimeout(<handler>, 100)). That's an ugly workaround but should give enough time to receive the version-update event

jdneo commented 3 years ago

Thank you @jrieken! I'll try the workaround tomorrow.

jrieken commented 3 years ago

We don't send an onDidClose-event to the extension host because there are still references to that document (one is from the openTextDocument-call).

Turns out this is unwanted. There is code in place which disposes references to deleted and moved text documents (and thereby freeing/closing the document). This code wasn't working properly for the delete case and assuming we do everything right in the "internal" this shouldn't happen anymore. At least with my fix it doesn't reproduce anymore. I will double check and update this issue

jrieken commented 3 years ago

This should be fixed, @jdneo please try with next insiders (and without a workaournd)

jdneo commented 3 years ago

I directly used the Code Actions in Java(which does exactly the same thing) to test the fix on my Windows machine. So far it looks good. I tried to remove the file both with and without saving, the workspace edit can correctly applied to the new generated file.

Below is the Insider version I used:

Version: 1.56.0-insider (user setup) Commit: b2d6cab998570dce35a80b0597758932cbfa7477 Date: 2021-04-29T14:45:56.276Z Electron: 12.0.4 Chrome: 89.0.4389.114 Node.js: 14.16.0 V8: 8.9.255.24-electron.0 OS: Windows_NT x64 10.0.19042

jrieken commented 3 years ago

Thanks - adding verified label based on you feedback. Feel free to come back if there is anything left