microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.41k stars 28.62k forks source link

Veto extension restart when there is an active notebook kernel #216922

Closed rebornix closed 1 week ago

rebornix commented 2 months ago

Does this issue occur when all extensions are disabled?: Yes/No

We might want to veto extension host auto restart when users are using a notebook kernel. Auto restarting will lead to data loss.

sandy081 commented 2 months ago

@rebornix Are you looking for some API support here? Do you have an idea how to veto here?

rebornix commented 2 months ago

@sandy081 it's the most accurate if it can be controlled by extensions (since only kernel extensions know if the kernel is stateful or not), but I think maybe we can start having an veto hook/api internally in renderer process, and the notebook component can veto whenever there is a notebook open and kernel selected. How does this sound to you?

sandy081 commented 2 months ago

We already have such an event in the workbench for the components to listen to and react

https://github.com/microsoft/vscode/blob/ca8cb6f637fc5ac820b581eb4115bc3917ba7548/src/vs/workbench/services/extensions/common/extensions.ts#L440-L444

rebornix commented 2 months ago

@sandy081 would you ever consider having an extension api, with which extensions can declare if they have intermediate state that can't be restored if EH restarts? Vetoing from the core can be a good start, but in practice only kernel extensions know if it should veto the restart. For example, GitHub Issue Notebook's kernel is stateless so it doesn't need to block the start. Jupyter should block if users are using a local kernel, but it should not if users are using remote kernels (as it knows how to auto reconnect).

Asking for a more granular control since if we veto if there is a kernel selected, it might mean a good amount of data science users will never get "auto restart" since their main editor is notebook.

sandy081 commented 2 months ago

would you ever consider having an extension api,

Actually we are trying to avoid that. But if it is needed, we can definitely work on it. It seems to be a problem not just during auto restart of extensions, but also when user restart extensions manually which can happen on profile switch / workspace trust / updating extensions.

Asking for a more granular control since if we veto if there is a kernel selected, it might mean a good amount of data science users will never get "auto restart" since their main editor is notebook.

Seems to make sense to me.

sandy081 commented 2 months ago

but I think maybe we can start having an veto hook/api internally in renderer process, and the notebook component can veto whenever there is a notebook open and kernel selected. How does this sound to you?

@rebornix Lets start with a veto when there is an open notebook. Should I go ahead and adopt the existing API in notebook land?

sandy081 commented 1 month ago

@rebornix I adopted notebooks for auto restarting of extension host - https://github.com/microsoft/vscode/pull/224178

Please take a look.

This disallow auto restarting of extension host when there is an open notebook editor.

I am planning to add similar fix for custom/webview editors.

sandy081 commented 2 weeks ago

Closing this and will open separate issue for custom editors

sandy081 commented 2 weeks ago

To verify:

  1. Enable the setting extensions.autoRestart
  2. Have a Notebook open
  3. Open extensions view and uninstall an extension that requires restarting extensions (You should see Restart Extensions button on uninstalling an extension)
  4. Switch the focus to any other window and come back to the existing window
  5. Make sure the extension host is not restarted.
  6. Close the opened notebook and do Step 4.
  7. Make sure the extension host is restarted.

@rebornix Can you please verify this.

rzhao271 commented 2 weeks ago

Step 5 fails. I'm uninstalling the Pull Requests extension for Step 3, and have a .ipynb notebook open for Step 2.