Sitelet https://web.archive.org/web/20230319185601/https://github.com/github/vscode-codeql/issues/983
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

Avoid restarting the query server while there are running queries #983

Open
aeisenberg opened this issue Oct 25, 2021 · 2 comments
Open
Labels
enhancement New feature or request good first issue Good for newcomers VSCode

Comments

@aeisenberg
Copy link
Contributor

aeisenberg commented Oct 25, 2021 •

Currently, if someone changes any query server settings, the query server is immediately stopped. Any running queries will complete with an error.

It should be possible to avoid restarting the server while there are running queries. The way that the query-server client is implemented, there are callbacks registered for any query or operation that is currently running. When completed, the callback is removed. Every time, after a callback is removed, we can check if the list of open callbacks is empty, and only then restart the server.

There are several potential edge cases to consider:

  1. If there is a zombie callback that is registered even though the thing it is registered for is no longer calling back, it will never be removed and the query server will never restart. This shouldn't happen, but we have nothing in place to ensure this.
  2. Consider the following pattern:
    1. User starts query1
    2. User changes a query server setting
    3. Query server restart is registered and is waiting for query1 to complete
    4. User starts query2. Query server restart is now waiting for both query1 and query2 to start
      It may be confusing for a user that their next query is using the old settings even though they requested a change.

For (1), perhaps we can set a timeout where if there are no callbacks triggered after X seconds, we assume they are all zombies and restart anyway. Long running queries will still invoke the callbacks on a regular basis, so this is safe.

For (2), I am less certain. Perhaps we can prevent starting new queries if a server restart is requested.


EDIT- After discussion with @adityasharad the simplest implementation that will suit are purposes is:

After editing any of the query server settings and there are server operations outstanding, open a popup saying something like: "Settings will not come into effect until the query server is restarted. Restart now? (This will cancel any currently executing queries.)" And add a button on the popup that triggers the restart.

The popup should be non-modal, allowing a user to potentially run new queries under the old settings. This is OK because it is clear what is happening.

@aeisenberg aeisenberg added the enhancement New feature or request label Oct 25, 2021
@adityasharad
Copy link
Contributor

adityasharad commented Oct 25, 2021 •

2 is tricky. I would prefer to keep the property that all running queries are using the same settings as configured in the settings file.

The goal is to avoid user surprise/confusion, so here are some more ideas that we could implement (selectively or all together):

  • Add warning text next to each query server setting, so the user knows that changing this will restart all running queries.
  • Pop a notification box with the above warning when a query server setting is changed.
  • Right now we cancel all existing operations, which is understandably annoying. Could we record which queries were running, and re-schedule them once the query server is restarted?

@aeisenberg
Copy link
Contributor Author

aeisenberg commented Oct 25, 2021 •

2 is tricky. I would prefer to keep the property that all running queries are using the same settings as configured in the settings file.

I agree, which is why we could consider preventing new query runs until the server is restarted (could be annoying, though).

  • Add warning text next to each query server setting, so the user knows that changing this will restart all running queries.

Maybe this is good enough for a simple fix. At least users wouldn't be surprised.

  • Pop a notification box with the above warning when a query server setting is changed.

Also a good idea. User can either wait until queries are done before clicking OK. Or click cancel to avoid updating the settings. Or they can just click OK immediately to cancel all queries right now.

  • Right now we cancel all existing operations, which is understandably annoying. Could we record which queries were running, and re-schedule them once the query server is restarted?

This is probably quite a bit of effort, especially since we can't simply re-run the same query file. The query or some of its dependencies may have changed, meaning the results would be surprising.

@aeisenberg aeisenberg added the good first issue Good for newcomers label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers VSCode
Projects
None yet
Development

No branches or pull requests

2 participants