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

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

Closed
rebornix opened this issue Jun 23, 2024 · 10 comments · Fixed by #227693
Closed

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

rebornix opened this issue Jun 23, 2024 · 10 comments · Fixed by #227693
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded

Comments

@rebornix
Copy link
Member

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
Copy link
Member

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

@sandy081 sandy081 added the info-needed Issue requires more information from poster label Jun 24, 2024
@rebornix
Copy link
Member Author

rebornix commented Jun 24, 2024

@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?

@rebornix rebornix added this to the On Deck milestone Jun 24, 2024
@sandy081
Copy link
Member

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

/**
* Fired before stop of extension hosts happens. Allows listeners to veto against the
* stop to prevent it from happening.
*/
onWillStop: Event<WillStopExtensionHostsEvent>;

@rebornix
Copy link
Member Author

@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 sandy081 changed the title Veto extension auto restart when there is an active notebook kernel Veto extension restart when there is an active notebook kernel Jun 27, 2024
@sandy081 sandy081 added feature-request Request for new features or functionality extension-host Extension host issues and removed info-needed Issue requires more information from poster labels Jun 27, 2024
@sandy081
Copy link
Member

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 sandy081 added the api label Jun 27, 2024
@sandy081 sandy081 modified the milestones: On Deck, July 2024 Jul 10, 2024
@sandy081
Copy link
Member

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
Copy link
Member

sandy081 commented Jul 29, 2024

@rebornix I adopted notebooks for auto restarting of extension host - #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 added a commit that referenced this issue Jul 29, 2024
#216922 do not auto restart when there is an open notebook
@sandy081
Copy link
Member

Closing this and will open separate issue for custom editors

@sandy081 sandy081 added the verification-needed Verification of issue is requested label Aug 26, 2024
@sandy081
Copy link
Member

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
Copy link
Contributor

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

@rzhao271 rzhao271 reopened this Aug 28, 2024
@rzhao271 rzhao271 added the verification-found Issue verification failed label Aug 28, 2024
@sandy081 sandy081 modified the milestones: August 2024, September 2024 Aug 28, 2024
sandy081 added a commit that referenced this issue Sep 5, 2024
@sandy081 sandy081 mentioned this issue Sep 5, 2024
sandy081 added a commit that referenced this issue Sep 5, 2024
@vs-code-engineering vs-code-engineering bot added the unreleased Patch has not yet been released in VS Code Insiders label Sep 5, 2024
@rzhao271 rzhao271 removed the verification-found Issue verification failed label Sep 5, 2024
@vs-code-engineering vs-code-engineering bot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Sep 6, 2024
@bpasero bpasero added the verified Verification succeeded label Sep 24, 2024
@vs-code-engineering vs-code-engineering bot locked and limited conversation to collaborators Oct 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders notebook verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants