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

Interrupt references code lens requests if user performs an action #60213

Closed
mjbvz opened this issue Oct 8, 2018 · 5 comments · Fixed by #60582
Closed

Interrupt references code lens requests if user performs an action #60213

mjbvz opened this issue Oct 8, 2018 · 5 comments · Fixed by #60582
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug javascript JavaScript support issues typescript Typescript support issues verified Verification succeeded
Milestone

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Oct 8, 2018

Repo

  1. With "typescript.referencesCodeLens.enabled": true
  2. Load file that has lots of references that take a while to resolve
  3. While the references are still coming in, request completions

Bug
We only return the completion result after the references requests are all resolved. This may take a little while.

Root Cause
VS Code calls resolveCodeLens for all visible references. This results in a number of requests being queued up against the TS server when we first load a file. Since VS Code's server communication queue is sequential and the TS server is single threaded, this means that any completions request must wait for all of the references requests to process before it can be sent

Potential fix would be to introduce the concept of a lo-priority request that other requests move in front of. Lo-pri requests already in progress would not be cancelled, but this would keep us from waiting until all code lenses are resolved before continuing.

@mjbvz mjbvz added bug Issue identified by VS Code Team member as probable bug help wanted Issues identified as good community contribution opportunities labels Oct 8, 2018
@mjbvz mjbvz self-assigned this Oct 8, 2018
@mjbvz mjbvz added typescript Typescript support issues javascript JavaScript support issues labels Oct 8, 2018
@ParkourKarthik
Copy link
Contributor

Hi @mjbvz , I would like to pick this up. Might need some guidance.

@ParkourKarthik
Copy link
Contributor

@mjbvz I tried loading a typescript file with more references after enabling "typescript.referencesCodeLens.enabled", but I was able to do simple user actions before all the references being loaded.
What kind of user action would make a completion request? Also it would be helpful if you could provide a sample ts with more references (may be in vscode source itself).

@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 9, 2018

To repo, just add a delay in https://github.com/Microsoft/vscode/blob/797161bd324200dcaf1cf297998a92b84c70ea9d/extensions/typescript-language-features/src/features/referencesCodeLens.ts#L19

The main point that needs to be updated to fix this issue is the request queue: https://github.com/Microsoft/vscode/blob/797161bd324200dcaf1cf297998a92b84c70ea9d/extensions/typescript-language-features/src/server.ts#L77

Instead of a generic push, the queue needs to have the concept of a 'priority', with normal priority requests moving in front of low priority ones like the references computations

@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 17, 2018

Merged in priority work but still needs code changes so we can use this in the references requests

@mjbvz mjbvz removed the help wanted Issues identified as good community contribution opportunities label Oct 17, 2018
@mjbvz mjbvz added this to the October 2018 milestone Oct 17, 2018
@mjbvz mjbvz modified the milestones: October 2018, November 2018 Oct 26, 2018
@mjbvz
Copy link
Contributor Author

mjbvz commented Nov 28, 2018

Use a larger file such as src/vs/editor/common/modes.ts for verification

mjbvz added a commit that referenced this issue Nov 28, 2018
This reverts commit dc47417.

We need finer control over how code lenses are resolved. This is required for #60213. Showing all references in the references code lense now requires using ts plugin instead
@mjbvz mjbvz closed this as completed in 42145f1 Nov 28, 2018
@RMacfarlane RMacfarlane added the verified Verification succeeded label Dec 6, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 12, 2019
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 javascript JavaScript support issues typescript Typescript support issues verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants