Skip to content

Correctly dispose TerminalManager even if terminals are not available #17876

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.

Sign up for GitHub

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

Jump to bottom
Merged
krassowski merged 2 commits into from
Sep 26, 2025
Merged

Correctly dispose TerminalManager even if terminals are not available #17876

krassowski merged 2 commits into from
Sep 26, 2025

Conversation

@krassowski
Copy link
Member

@krassowski krassowski commented Sep 11, 2025 •
edited
Loading

References

Fixes #17875

Code changes

  • Fixes typing
  • Fixes usage

User-facing changes

None

Backwards-incompatible changes

None

@jupyterlab-probot
Copy link

jupyterlab-probot bot commented Sep 11, 2025

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@krassowski krassowski added the bug label Sep 11, 2025
jtpio
jtpio reviewed Sep 11, 2025
View reviewed changes
packages/services/src/terminal/manager.ts

// Start polling with exponential backoff.
this._pollModels = new Poll({
const pollModels = (this._pollModels = new Poll({
Copy link
Member

@jtpio jtpio Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an alternative to avoid changing the types could be to move the block that checks this.isAvailable() a bit below, before setting the this._ready promise which then starts the polls?

Copy link
Member Author

@krassowski krassowski Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could do that but it feels like updating the types is fine:

  • it is a private variable
  • if a user/extension called refreshRunning() with poll initialized it could have unexpected effects; with this PR as is it would just do nothing.

Copy link
Member Author

@krassowski krassowski Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtpio is changing the types a big concern? If so I can rework this PR.

Copy link
Member

@jtpio jtpio Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's likely fine.

Probably we could also consider instantiating a NoopManager at the plugin level when the terminals are not available?

Something like the following in:

jupyterlab/packages/services-extension/src/index.ts

Lines 257 to 278 in 4cdd285

/**
* The terminal manager plugin.
*/
const terminalManagerPlugin: ServiceManagerPlugin<Terminal.IManager> = {
id: '@jupyterlab/services-extension:terminal-manager',
description: 'The terminal manager plugin.',
autoStart: true,
provides: ITerminalManager,
optional: [IServerSettings, IConnectionStatus],
activate: (
_: null,
serverSettings: ServerConnection.ISettings | undefined,
connectionStatus: IConnectionStatus | undefined
): Terminal.IManager => {
return new TerminalManager({
serverSettings,
standby: () => {
return !connectionStatus?.isConnected || 'when-hidden';
}
});
}
}; 

if (!Terminal.isAvailable()) {
  return new TerminalManager.NoopManager();
}

Copy link
Member Author

@krassowski krassowski Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, but this might be a potentially breaking change - see my comment in #17930 which I opened to explore this as a follow-up (since I would like to backport this PR to 4.4.x).

@krassowski krassowski added this to the 4.4.x milestone Sep 15, 2025
@krassowski krassowski mentioned this pull request Sep 26, 2025
Draft
jtpio
jtpio approved these changes Sep 26, 2025
View reviewed changes
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@krassowski krassowski merged commit 3012f01 into jupyterlab:main Sep 26, 2025
86 of 87 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Sep 26, 2025
@krassowski @meeseeksmachine
Backport PR jupyterlab#17876: Correctly dispose TerminalManager eve…
1d71f67 
…n if terminals are not available
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 26, 2025
Merged
@krassowski krassowski deleted the fix-disposal branch September 26, 2025 14:34
krassowski added a commit that referenced this pull request Sep 26, 2025
@meeseeksmachine @krassowski
Backport PR #17876: Correctly dispose TerminalManager even if termi…
80d2a2c 
…nals are not available (#17934)

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtpio jtpio jtpio approved these changes

Assignees

@krassowski krassowski

Labels

bug pkg:services

Projects

None yet

Milestone

4.4.x

Development

Successfully merging this pull request may close these issues.

ServiceManager.dispose() fails if terminals are not available

2 participants

@krassowski @jtpio