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

Fix o# getting stuck after undoing delete/rename/move transient files #1308

Closed

Conversation

SirIntruder
Copy link
Contributor

  1. Have simple MSBuild workspace with one .csproj
  2. Add file to path "src/Foo.cs"
  3. Delete file on path "src/Foo.cs"

Path "src/Foo.cs" is now "poisoned" because workspace removed the created document, but BufferManager didn't remove it from its internal collections of transient documents.

Adding anything to that path results in documend addition failing with "KeyAlreadyExists" exception, and from the user's perspective, there will be no language features on this path until omnisharp is restarted, and the rest of the workspace will treat contents on that path as non existing.

PR forces BufferManager.OnWorkspaceChanged to properly update its collections on DocumentRemove event.

I expanded existing test, which was file creation only, to test create-delete-create. I could copy that test and add addition asserts, just didn't want to dent on build time with seting up new o# test host (somebody correct me if this is too paranoid 😄).

Create-delete-create is a pretty artificial case, but similar issue can happen in many situations, as in undoing renames, accidental drag'n'drops and switching branches etc.

@rchande
Copy link

rchande commented Dec 11, 2018

@SirIntruder Is this still active? Or superseded by #1357 ?

@SirIntruder
Copy link
Contributor Author

By #1330, actually. This PR would have prevented dangling references to deleted documents, but with #1330 those dangling references are just overwritten over and no longer cause "KeyAlreadyExist" exceptions.

I am finished with this PR, it fixes the underlying issue as it is. But since the underlying issue currently doesn't manifest itself, it's ok to close this just to avoid change noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants