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

<debug_request> publish the <status:idle> while an <execute_request> is running #799

Open
mizililo opened this issue Nov 17, 2021 · 5 comments

Comments

@mizililo
Copy link

mizililo commented Nov 17, 2021

In ipykernel 5.5.6,both <control_stream> and <shell_stream> put msgs into <msg_queue> and process the msg with <schedule_dispatch>. Although both <dispatch_control> and <dispatch_shell> publish the <status: busy/idle>,there are no conflicts.

When it comes to ipykernel 6.5.0, <control_stream> put msgs into <control_queue> and process the msg with <dispatch_control>, while shell_stream still works in the old way. This means <process_control> and <dispatch_shell> will publish the <status: busy/idle> in the same time.

I found this problem in jupyterlab when i executed a cell(sleep 600s) in an ipykernel with <metadata: {debugger: True}>,i switched to another ipynb and switched back. The cell was still running, but the kernel execution_state became idle. Then i found the ipykernel received multi <debug_requests> and published the status to idle.

As https://jupyter-client.readthedocs.io/en/latest/messaging.html said "Debug requests and replies are sent over the control channel to prevent queuing behind execution requests". It confuses me that why control_requests affect the kernel's execution_state? Should we use another state like control_state?

@minrk
Copy link
Member

minrk commented Nov 17, 2021

Until the debugger, all requests on both control and shell were handled in the say way - where one request could be processed at a time, and thus the kernel was always 'busy' or 'idle', regardless of which queue the task came from. Control being a separate queue does not mean that control requests don't block in the same way that shell requests do, nor that they are concurrent. Just that they jump to the head of the line when the kernel is ready to handle the next request, whichever queue it may come from. However, debug state being concurrent does complicate things. I'm not quite sure what the right answer is there.

@mizililo
Copy link
Author

mizililo commented Nov 17, 2021

Until the debugger, all requests on both control and shell were handled in the same way.

In previous versions like 5.5.6, it's true and all requests are put into <msg_queue> by <schedule_dispatch> and processed in order by <dispatch_queue> -> <process_one>
While in latest version, msgs from <crontrol_stream> are put into <control_queue> by <dispatch_control> and processed by <poll_control_queue> -> < process_control>. This Actually Make shell_request / control_request Work Concurrently.
I think the pollers(<poll_control_queue> and <dispatch_queue>) do cause the bug.

@mizililo
Copy link
Author

In short, previous versions use one poller to get msg from msg_queue, while the latest version uses two pollers to get msg from two queues, and this causes two dispatchers to process and publish the status concurrently

@JohanMabille
Copy link
Contributor

I see two options to fix this issue (but there are probably others):

  • either we consider that a kernel busy means something is running on the shell channel, the control being reserved for short transactions for, well, "controlling" the kernel. In that case, the kernel should emit status messages only when receiving a request on the shell channel
  • or we add a field in the status message so that clients can know which channel is busy. This field can be a list of channels so it's easy to handle kernels where control and shell are run by the same thread, or we can have a specific value "all" to indicate that all channels are busy.

I find the last solution more flexible (it allows to add more costly request on the control channel in the future), and that would be a backward compatible change in the protocol.

@mizililo
Copy link
Author

mizililo commented Dec 7, 2021

#819

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

No branches or pull requests

3 participants