ensure qt zmq streams starts in a clean state #307
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adding a listener on stream.FD won’t reliably wake the eventloop if there are already events waiting to be processed. We must check socket.EVENTS and process any waiting events before zmq.FD will be written-to again.
|
Thanks for the quick response Min! I confirm this PR solves the problem I reported in pyzmq. |
willingc
approved these changes
Feb 12, 2018
ipykernel/eventloops.py
Outdated
| @@ -41,6 +41,11 @@ def process_stream_events(): | |||
| fd = stream.getsockopt(zmq.FD) | |||
| notifier = QtCore.QSocketNotifier(fd, QtCore.QSocketNotifier.Read, kernel.app) | |||
| notifier.activated.connect(process_stream_events) | |||
| # there may already be events waiting, | |||
There was a problem hiding this comment.
@minrk Double checking my understanding and perhaps tweaking the comment.
there may already be unprocessed events waiting.
these events will not wake zmq's edge-triggered FD
since edge-triggered notification only occurs on new i/o activity.
process all the waiting events immediately
so we start in a clean state ensuring that any new i/o events will notify.
with a zero-timeout single-shot timer (closest I could get to .call_soon()) rather than calling it immediately, which might block in unexpected ways by processing events while we are still hooking things up.
|
Updated to schedule the first call on the eventloop via a QTimer, rather than making a potentially long-running blocking call immediately which may be a bit early. |
|
@minrk Looks good. Tested locally with @ccordoba12's setting in the config file. Tests pass and gui appears. Nicely done. |
|
releasing soon 4.8.2 ? |
|
Yep, please. We're already getting bug reports about this: |
|
Just released 4.8.2 with this patch. |
|
conda-forge pending |
|
Thanks a lot Min!
El 19/02/18 a las 11:21, Min RK escribió:
…
Just released 4.8.2 with this patch.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#307 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAWS7fxa1TXRrahTMaIdeQ4V7QKCLboyks5tWZ91gaJpZM4SBCXu>.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
adding a listener on stream.FD won’t reliably wake the eventloop if there are already events waiting to be processed. We must check socket.EVENTS and process any waiting events before waiting on zmq.FD, or we will never wake.
This fixes the bug reported in zeromq/pyzmq#1140
It's caused by updating pyzmq to version 17,
where the tornado eventloop integration also relies on zmq.FD rather than zmq_poll, which allows the initial state of the FD to be consumed (by tornado) while there are events waiting to be processed (because tornado is paused indefinitely).
I think there's something fragile in our multi-eventloop integration because we aren't explicitly advancing the tornado eventloop at all when a GUI eventloop is running, only our
kernel.do_one_iterationcallback. This means that any callbacks registered directly with tornado/asyncio will not run while eventloop integration is enabled.cc @ccordoba12 who reported the issue. Can you confirm this fixes it for you?