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

call _create_connected_socket to instantiate _control_socket in Kerne… #456

Merged
merged 1 commit into from
Jul 9, 2019
Merged

Conversation

JohanMabille
Copy link
Member

@JohanMabille JohanMabille commented Jul 9, 2019

…lManager

This PR fixes a bug introduced with #447; now that IOLoopKernelManager exposes a decorator of connect_control, this method is wrongly called by the KernelManager when creating its internal control socket: https://github.com/jupyter/jupyter_client/blob/master/jupyter_client/manager.py#L199.

This results in creating a ZMQStream instead of a regular ZMQSocket as it was the case before, when connect_control of ConnectionFileMixin was directly called. I'm not familiar with pyzmq but this prevents the shutdown_request message to reach the kernel, leading to always send a signal to kill it.

cc @SylvainCorlay @rgbkrk

@SylvainCorlay
Copy link
Member

Thanks @JohanMabille. I will merge when all is green.

@SylvainCorlay
Copy link
Member

So the repo2docker builds seem to have queued up and they take a lot of time. This may delay the merge by a bit.

@SylvainCorlay SylvainCorlay merged commit 43c9c2f into jupyter:master Jul 9, 2019
@SylvainCorlay
Copy link
Member

@JohanMabille do you want to open the backport PR to 5.x?

@JohanMabille JohanMabille deleted the control branch July 9, 2019 16:10
@JohanMabille
Copy link
Member Author

Yes

SylvainCorlay added a commit that referenced this pull request Jul 9, 2019
[backport #456 to 5.x] call _create_connected_socket to instantiate _control_socket in KernelManager
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 this pull request may close these issues.

2 participants