vscode-codeql icon indicating copy to clipboard operation
vscode-codeql copied to clipboard

Avoid restarting the query server while there are running queries

Open aeisenberg opened this issue 4 years ago • 2 comments

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 avatar Oct 25 '21 21:10 aeisenberg

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?

adityasharad avatar Oct 25 '21 21:10 adityasharad

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 avatar Oct 25 '21 21:10 aeisenberg