-
-
Notifications
You must be signed in to change notification settings - Fork 396
publish IOPub from a background thread #85
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
Conversation
e056263 to
20221e9
Compare
|
I think this should be ready to go, now. The one thing I'm not thrilled about is the workaround of the fork-safety check in |
|
I made the fork-safe check optional in jupyter/jupyter_client#107, so we can clean that up later when we bump dependencies. Since this has been ready to go for some time now, I'll merge before too long unless someone wants to do some review. |
|
Sorry for leaving this unreviewed so long. I will try to have a careful look at it either this evening or tomorrow (just about to head home before the rain gets too hard). |
ipykernel/iostream.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I'm overriding __getattr__ and __setattr__, I like to check for attempts at __special__ attribute access, and make them behave as normal; otherwise things can get quite confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__methods__ don't passthrough and Socket attr access is deprecated. I didn't find any actual use of it, so it may be overly cautious, though it may be that ipyparallel will rely on it for some transition.
|
Re: BackgroundSocket I didn't deprecated the socket API because I don't really have an idea for a better one. If nothing else, it's going to be an object with a send method that takes a message, so I'm not sure how different it's going to be from the socket API that we need to keep for backward compatibility. Do you have any ideas on that point? |
|
Fair enough. Is it worth deprecating the shortcut access to all of the socket methods besides send? What actually uses those? |
I expect so. I'll take a stab at that, and see what might be using them, if anything. |
removes need to call `sys.stdout.flush` during blocking loops for backward-compatibility, a BackgroundSocket is passed around that transparently sends messages via the IOPub thread.
add_timeout isn't threadsafe, use add_callback to schedule timeout
relevant for kernel subclasses
not present in pyzmq's bundled IOLoop
ipykernel/iostream.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think __setattr__ should also be doing a check like this (though it should call super, rather than raising AttributeError).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and super'd
there appear to be no uses of this in ipykernel, at least.
- remove now-trivial IOStream._check_mp_mode - clarify placeholder comment
publish IOPub from a background thread
removes need to call
sys.stdout.flushduring blocking loops for backward-compatibility, a BackgroundSocket is passed around that transparently sends messages via the IOPub thread.I had a hell of a time debugging weird hangs with multiprocessing, but they seem to be gone, now, thanks to a well-placed threading.Event.
closes jupyter/notebook#700
closes #77