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 the Control channel in a separate thread #447

Closed
vladimirlagunov opened this issue Oct 7, 2019 · 9 comments · Fixed by #585
Closed

Handle the Control channel in a separate thread #447

vladimirlagunov opened this issue Oct 7, 2019 · 9 comments · Fixed by #585

Comments

@vladimirlagunov
Copy link

Hello,

I’d created a similar discussion in Jupyter Google Groups but looks that responsible people from IPython kernel does not participate in groups.

Brief explanation: I tried to implement a tunnel for the pydev debugger through notebook’s websocket and using Comm. Unfortunately, Comm and cell execution shares the same thread, so when debugger suspends the cell’s code it also suspends the tunnel. Anyway, I'd succeeded to implement it by moving a Control channel in a separate thread using monkey-patching but it doesn't look like a production-ready solution.

I propose to move handling of the Control channel in a separate thread and add an analog of Comm for that thread. It could be used for tunneling streams of a debugger or for collecting metrics at runtime.

A rough algorithm of handling each message in a separate thread:

  • If the message can be handled in the Control thread then handle it immediately. Example of methods that can be handled here: kernel_info_request, complete_request, Comm for Control.
  • Otherwise the message is being re-queued into main thread’s event loop and then it being handled like in a current implementation.

Do you see any obvious pitfalls around handling the Control channel in a separate thread? If I’ll prepare a patch with acceptable code quality, will you merge it?

@impact27
Copy link
Contributor

I think that a new channel that is explicitly for threaded operation would be better (ThreadedControl?), so there is no ambiguity as to if the message is handled on the main thread or in the dedicated thread.

@vladimirlagunov
Copy link
Author

@impact27 sure, it would be a much cleaner architecture. However, if a new channel appeared in ipykernel, it would also require support of the channel in Jupyter Notebook server and its analogs. Such massive change hardly possible to implement everywhere.

@impact27
Copy link
Contributor

impact27 commented Dec 18, 2019

It could be an optional channel. It could be connected only upon request.

@vladimirlagunov
Copy link
Author

An optional channel still should be proxied through the Jupyter Notebook server, and now it doesn't know anything about an optional channel. Talking about Jupyter hosted on a remote server, usually it's possible to connect only to Jupyter Notebook server, all other listening TCP ports could be filtered by a firewall.

@ccordoba12
Copy link
Member

Do you see any obvious pitfalls around handling the Control channel in a separate thread?

@blink1073, @SylvainCorlay, what do you think about this? We really need something like this in Spyder to not create our own channel and avoid problems like spyder-ide/spyder#13288, which we've been unable to solve after months of trying it.

@blink1073
Copy link
Contributor

I think as long as the control channel isn't modifying kernel state, it should be fine to run it in a separate thread.

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Aug 1, 2020

@ccordoba12 @blink1073 yes! We definitely need to run the control channel in its own thread, for many reasons.

Actually

  • we now recommend it in the documentation for the jupyter messaging protocol: "we recommend running the control channel in a separate thread from the shell channel, so that e.g. shutdown or debug messages can be processed immediately".
  • this is what we do in xeus-python and other xeus-based kernels by default.

The plan is to make that change in ipykernel, and also to implement the new Jupyter Debugger Protocol in ipykernel (and not just in xeus-python). I would love to see spyder also be a front-end to that protocol!

@SylvainCorlay
Copy link
Member

@ccordoba12 @vladimirlagunov FYI

PR #585 splits control into a separate thread
PR #597 by @JohanMabille (still a draft) comes on top of #585 implements the Jupyter debug protocol.

@ccordoba12
Copy link
Member

Thanks a lot @SylvainCorlay for the ping and your work on this!

Pinging @impact27 in case he wants to take a look at the debugger implementation in PR #597.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants