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

Handle disconnect messages from the terminal #6339

Merged
merged 5 commits into from May 13, 2019

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 11, 2019

References

Fixes #5061

Code changes

Adds internal handling of the disconnect message from the server-side terminal. We dispose of the connection and the widget when we get this message.

User-facing changes

More consistent behavior with other mulit-terminal applications (e.g. Gnome Terminal, ConEmu).

Backwards-incompatible changes

The Terminal widget now requires a session as part of the ctor arg and session is a read-only property.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented May 11, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073 blink1073 added this to the 1.0 milestone May 11, 2019
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Looks good to me, and fixes the immediate problem. When testing this, I also noticed that when shutting down the terminal from the "running" bar, the terminal doesn't seem to reflect the shutdown. It stops accepting input, but doesn't show any "Finished" message. Maybe out of scope for this fix, but surprising behavior nonetheless.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented May 11, 2019

I am going to look a bit closer. I think we might need to keep the socket open on shutdown until we get the disconnect message. https://github.com/jupyter/terminado/blob/master/terminado/websocket.py#L98

@blink1073 blink1073 changed the title Handle disconnect messages from the terminal [wip] Handle disconnect messages from the terminal May 11, 2019
@blink1073 blink1073 changed the title [wip] Handle disconnect messages from the terminal Handle disconnect messages from the terminal May 13, 2019
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented May 13, 2019

@ian-r-rose, updated. Note the new comments in the PR description.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 13, 2019

The new disposal on disconnect is cool. I kind of like it, but I'm wondering what other folks think about it. Most of our other widgets don't auto-dispose on disconnects.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

LGTM

@ian-r-rose ian-r-rose merged commit 8a5a4f5 into jupyterlab:master May 13, 2019
9 checks passed
@blink1073 blink1073 deleted the terminal-zombies branch Jun 2, 2019
@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants