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

workspace.applyEdit allows 'concurrent' calls to succeed #54773

Closed
grork opened this issue Jul 20, 2018 · 2 comments
Closed

workspace.applyEdit allows 'concurrent' calls to succeed #54773

grork opened this issue Jul 20, 2018 · 2 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority verified Verification succeeded
Milestone

Comments

@grork
Copy link

grork commented Jul 20, 2018

In Stable, when calling applyEdit multiple times before the first one is complete, subsequent calls are rejected, until the first edit is completes. This allows you to make the assertion that if an applyEdit succeeds, there should be one, and only one, onDidChangeTextDocument change event containing the edits that were applied.

This is not true in Insiders -- both applyEdit calls will succeed, and there will be multiple onDidChangeTextDocument events raised.

Attached is an example extension that has one command -- minibash.concurrentBash that will show the repro.

Expected Debug console output w/ Stable:

1: Applying Edit: 0,0, text: 'O'
2: Applying Edit: 0,0, text: 'DRENG'
Doc changed with 1 changes
1: Applied Edits
2: Applied Edits
2: Failed to apply edits
Failed

Output w/ Insiders:

1: Applying Edit: 0,0, text: 'M'
2: Applying Edit: 0,0, text: 'RRGAOCOQ'
Doc changed with 1 changes
Doc changed with 1 changes
1: Applied Edits
2: Applied Edits
saw multiple edits
saw multiple edits
Failed

To run this:

  1. open the extension sources, and F5
  2. create a new empty document (or open existing one)
  3. Run Minibash: Concurrently Bash
    Minibash.zip

Note, that if you remove the return false; on line 69 in doEditsUntilBad, you'll see that Insiders will happly apply 1000 edits. Stable does not; it immediately fails on the second edit (As expected)

@alexdima alexdima self-assigned this Jul 23, 2018
@alexdima alexdima added this to the July 2018 milestone Jul 23, 2018
@alexdima alexdima added the important Issue identified as high-priority label Jul 23, 2018
@alexdima
Copy link
Member

@grork Thank you for the repro.

I can reproduce. This issue was introduced with 54a6ec8#diff-ffd7342f51e8e5d64bad8622e6f5a48e

The gist of it is that the version id is checked and then asynchronously the edits are applied. We should check the version id right before applying the edits...

@alexdima alexdima added the bug Issue identified by VS Code Team member as probable bug label Jul 24, 2018
@jrieken
Copy link
Member

jrieken commented Jul 24, 2018

Thanks @alexandrudima

@octref octref added the verified Verification succeeded label Aug 2, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Sep 7, 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 important Issue identified as high-priority verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants