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

Conversation

@JohanMabille
Copy link
Member

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

This comment has been minimized.

Copy link
Member

SylvainCorlay commented Jul 9, 2019

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

@SylvainCorlay

This comment has been minimized.

Copy link
Member

SylvainCorlay commented Jul 9, 2019

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
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SylvainCorlay

This comment has been minimized.

Copy link
Member

SylvainCorlay commented Jul 9, 2019

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

@JohanMabille JohanMabille deleted the JohanMabille:control branch Jul 9, 2019
@JohanMabille

This comment has been minimized.

Copy link
Member Author

JohanMabille commented Jul 9, 2019

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
Projects
None yet
2 participants
You can’t perform that action at this time.