KernelManagers don't use zmq eventloop properly #967

Closed
minrk opened this Issue Nov 2, 2011 · 2 comments

Comments

Projects
None yet
1 participant
@minrk
Member

minrk commented Nov 2, 2011

The KernelManagers use sockets and tornado handlers directly via add/drop_io_state, thus essentially duplicating the ZMQStream objects that handle the state triggering, queuing, etc. already. Approximately all of the private methods on these channels are redundant with code already in ZMQStream.

Also, each channel should probably not be in a separate thread, they should share one ioloop instance between them, and it should be possible for this loop to be run in the main thread if so desired. The most apparent problem this causes (a minor one) is that stopping the channels can take a full second if there is no traffic on the network, which is a long time. I cannot think of any benefit to the channels running in their own threads, as they do now.

This is a low-ish priority, because the code we have works, but it should definitely be fixed.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 3, 2011

Member

I should also note that the polling logic in the HB channel is unnecessarily complicated, and actually wrong in some places. There is a comment interpreting poll() returning an empty list as a zeromq bug, which it certainly is not, and the code appears to follow this misunderstanding. Since this bug is actually critical to getting the terminal-frontend working, I will at least fix that part over in #864.

Member

minrk commented Nov 3, 2011

I should also note that the polling logic in the HB channel is unnecessarily complicated, and actually wrong in some places. There is a comment interpreting poll() returning an empty list as a zeromq bug, which it certainly is not, and the code appears to follow this misunderstanding. Since this bug is actually critical to getting the terminal-frontend working, I will at least fix that part over in #864.

minrk added a commit that referenced this issue Nov 3, 2011

Fixes to the heartbeat channel
* The heartbeat channel had some erroneous zeromq logic, and entirely False comments (as described in #967).  This has been fixed.

* KernelManager.is_alive() checks if the hb_channel is running if
the kernel is not owned, rather than always returning True.

* BlockingKM's hb_channel has been relaxed to 1s polling, because replies are not
reliably much faster than that.  There are occasional >0.5s outlier responses.

minrk added a commit that referenced this issue Nov 7, 2011

Fixes to the heartbeat channel
* The heartbeat channel had some erroneous zeromq logic, and entirely False comments (as described in #967).  This has been fixed.

* KernelManager.is_alive() checks if the hb_channel is running if
the kernel is not owned, rather than always returning True.

* BlockingKM's hb_channel has been relaxed to 1s polling, because replies are not
reliably much faster than that.  There are occasional >0.5s outlier responses.

minrk added a commit that referenced this issue Nov 21, 2011

Fixes to the heartbeat channel
* The heartbeat channel had some erroneous zeromq logic, and entirely False comments (as described in #967).  This has been fixed.

* KernelManager.is_alive() checks if the hb_channel is running if
the kernel is not owned, rather than always returning True.

* BlockingKM's hb_channel has been relaxed to 1s polling, because replies are not
reliably much faster than that.  There are occasional >0.5s outlier responses.
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 27, 2011

Member

closed by PR #1030

Member

minrk commented Nov 27, 2011

closed by PR #1030

@minrk minrk closed this Nov 27, 2011

minrk added a commit that referenced this issue Nov 29, 2011

Fixes to the heartbeat channel
* The heartbeat channel had some erroneous zeromq logic, and entirely False comments (as described in #967).  This has been fixed.

* KernelManager.is_alive() checks if the hb_channel is running if
the kernel is not owned, rather than always returning True.

* BlockingKM's hb_channel has been relaxed to 1s polling, because replies are not
reliably much faster than that.  There are occasional >0.5s outlier responses.

minrk added a commit that referenced this issue Dec 6, 2011

Fixes to the heartbeat channel
* The heartbeat channel had some erroneous zeromq logic, and entirely False comments (as described in #967).  This has been fixed.

* KernelManager.is_alive() checks if the hb_channel is running if
the kernel is not owned, rather than always returning True.

* BlockingKM's hb_channel has been relaxed to 1s polling, because replies are not
reliably much faster than that.  There are occasional >0.5s outlier responses.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014

Fixes to the heartbeat channel
* The heartbeat channel had some erroneous zeromq logic, and entirely False comments (as described in #967).  This has been fixed.

* KernelManager.is_alive() checks if the hb_channel is running if
the kernel is not owned, rather than always returning True.

* BlockingKM's hb_channel has been relaxed to 1s polling, because replies are not
reliably much faster than that.  There are occasional >0.5s outlier responses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment