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

Settings search with "bad" internet is blocked #42741

Closed
alexdima opened this issue Feb 1, 2018 · 6 comments
Closed

Settings search with "bad" internet is blocked #42741

alexdima opened this issue Feb 1, 2018 · 6 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug settings-editor VS Code settings editor issues verified Verification succeeded
Milestone

Comments

@alexdima
Copy link
Member

alexdima commented Feb 1, 2018

Simulate "bad" internet:

image

Search for settings in the Settings Editor. It will search for parts of your query, then block for 20s, then search for the rest of the query.

fyi @egamma

@sandy081 sandy081 added bug Issue identified by VS Code Team member as probable bug settings-editor VS Code settings editor issues labels Feb 1, 2018
@sandy081 sandy081 added this to the January 2018 milestone Feb 1, 2018
@sandy081
Copy link
Member

sandy081 commented Feb 1, 2018

@roblourens I provided a fix here to separate local search and remote search. So when remote search is blocked or taking too much of time, local search can still run and show results.

But, please take a look at the following few issues I noticed with remote search

  1. Remote search is not cancelled when I cleared the search input
  2. Showing outdated results. See in the following video, remote search is showing outdated results when filter string is cleared.
  3. Can we have a timeout for remote search? For eg., cancel the requests if they take more than one second?

kapture 2018-02-01 at 22 10 06

@roblourens
Copy link
Member

We do have a timeout of 5 seconds, but @alexandrudima bypassed it because it's implemented by XHR. So I think that isn't a good test.

@roblourens
Copy link
Member

But I'll look at @sandy081's change

roblourens added a commit that referenced this issue Feb 1, 2018
…. It should simply debounce the remote search, each new instance cancelling the previous if it hasn't completed
@roblourens
Copy link
Member

roblourens commented Feb 1, 2018

I see that the earlier progress bar change caused the local search to wait on the remote search, and the new change decouples them, which is correct.

I debugged some and realized that I'm doing something slightly wrong, in that the remote search is debounced, but each new search waits on the old one to complete. It should actually fire immediately and cancel the previous search if it has not completed yet. I didn't realize that the ThrottledDelayer is queuing the promises, makes sense though.

I have a change to make the ThrottledDelayer simply not wait on the search to complete. But, I don't think I want to check it in now because the impact isn't that bad, with the remote search timeout. And it isn't noticeable under normal network conditions. The change is here: 0dbb7a0

But @sandy081 I've tried a lot and I can't repro what you show where the results show up after the query has been cleared. That shouldn't happen, due to the search being canceled when the query is cleared.

@sandy081
Copy link
Member

sandy081 commented Feb 2, 2018

@roblourens Good.

Opened a separate issue to improve this throttled behaviour - #42814

Closing this one as decoupling local and remote searches fixes this issue.

@sandy081 sandy081 closed this as completed Feb 2, 2018
@alexdima alexdima added the verified Verification succeeded label Feb 2, 2018
@alexdima
Copy link
Member Author

alexdima commented Feb 2, 2018

Thank you!

@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 19, 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 settings-editor VS Code settings editor issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants