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

Changed zmqContext to not reuse instances #448

Merged
merged 2 commits into from
Jun 26, 2019
Merged

Conversation

MSeal
Copy link
Contributor

@MSeal MSeal commented Jun 13, 2019

Followup to #438 to provide the same thread / process safety as the kernel manager used by nbconvert. This has a small memory overhead to avoid parent processes from deadlocking when operating with concurrency.

@MSeal MSeal requested a review from mpacer June 13, 2019 04:52
@mpacer
Copy link
Member

mpacer commented Jun 13, 2019

Is there a way to add tests to verify that this behaviour continues (basically a uniqueness check for the accessible zmqContext instances?

@MSeal
Copy link
Contributor Author

MSeal commented Jun 13, 2019

I'm not sure a uniqueness check is a useful test. I looked into each of the classes that got touched and found that the KernelManager already has strong test coverage on the calls that use zmq, including parallelization. The MultiKernelManager was missing test coverage however so I added some parallelism sanity tests.

@rgbkrk rgbkrk requested a review from minrk June 17, 2019 16:53
@rgbkrk
Copy link
Member

rgbkrk commented Jun 17, 2019

This seems right, but since I'm not as well versed in pyzmq I've added @minrk to check it out in case there's anything we're missing.

@rgbkrk
Copy link
Member

rgbkrk commented Jun 26, 2019

Now that I've had a better chance to look over this, I'm happy to bring it in.

@rgbkrk rgbkrk merged commit f083570 into master Jun 26, 2019
@rgbkrk rgbkrk deleted the zmqContextIsolation branch June 26, 2019 16:42
@minrk
Copy link
Member

minrk commented Jun 28, 2019

Sorry for being slow, but 100% okay with this. This does mean that the zmq context is not shared across threads, which is perhaps atypical. The the only functional result should be that:

  1. there will be a couple more C++ io threads running to handle messages (probably doesn't matter), and
  2. inproc can't be used, but I think there are other reasons why inproc can't be used already, mainly that it's only in-process, so can't talk to out-of-process kernels.

The alternate fix would have been to ensure that zmq.Context.instance() is fork-safe, which I honestly thought it was, but it totally isn't. I should have known this, since I wrote it!

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.

4 participants