-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Fix for the terminal shutdown issue #4180
Conversation
This patch prevents creation of a new terminal when handling websocket handshaking request. The default behavior of `terminado.NamedTermManager` is to automatically start a new terminal in response to a websocket handshake, which prevents a terminal from being properly shut down in JupyterLab as reported in this [issue](jupyterlab/jupyterlab#5061).
I consider it a nice hidden feature that you can create named terminals just by going to the URL, so it would be a shame to lose it. Do you know if there's an issue open about it on xtermjs? I think it should be technically possible for the server side to close down the connection and indicate to xtermjs that it's deliberately being closed. |
@takluyver I didn't find a relevant issue on xtermjs. In xtermjs, the
Implementing such a feature in xtermjs will require lots of testing. |
I agree if the creation is triggered by a http |
I agree with this sentiment: starting a terminal should be an HTTP request, not a websocket request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
What would folks think about a special named path like |
That seems reasonable to be, but only if it redirected you to a new numbered terminal url. |
Seems reasonable to me too. Maybe add to jupyter_server as well 😉 |
I feel like I'm missing some context. How is this different than a |
This would be a URL you visit in your browser, a GET that gives a 302 to a named terminal URL. |
This PR is aimed to fix the problem of Zombie terminal as reported in issue 5061 of JupyterLab. It adds checking for the parameter of websocket request against existing terminals in order to avoid unexpected creation of terminals. According to the issue report, the
xtermjs
client tries to recreate the websocket connection after termination of the shell process, which then triggers the creation of another terminal (shell process) on the server.The issue can also be "fixed" in the upstream project
terminado
, in which the default behavior ofNamedTermManager
is to automatically start a new terminal in response to a websocket handshake. However, the fix proposed in this PR is cleaner and more efficient.