use ROUTER/DEALER socket names instead of XREP/XREQ #758

Merged
merged 1 commit into from Sep 6, 2011

Conversation

Projects
None yet
2 participants
@minrk
Member

minrk commented Sep 4, 2011

This is principally a big find/replace, but also adjusts the import-check for pyzmq/zmq versions in IPython.zmq and IPython.parallel (allowing for two-digit entries in version comparisons, in case of possible 2.1.10 release).

XREP/XREQ are aliases for ROUTER/DEALER in 0MQ 2.1.x, so there is actually no change in behavior. However, these sockets continue to exist in 3.0 under the ROUTER/DEALER names only. The XREP/XREQ protocols change some in 3.0, and won't work properly with current IPython.

It is likely that once 3.0 is stable (and pyzmq supports it better), we will want to move some sockets back to the new XREP/XREQ, but this PR should make IPython safe through libzmq-3.x.

@ellisonbg

View changes

IPython/zmq/kernelmanager.py
- self.socket = self.context.socket(zmq.XREQ)
- self.socket.setsockopt(zmq.IDENTITY, self.session.session)
+ self.socket = self.context.socket(zmq.DEALER)
+ # self.socket.setsockopt(zmq.IDENTITY, self.session.session)

This comment has been minimized.

@ellisonbg

ellisonbg Sep 6, 2011

Member

Why are these lines commented out? Should we not be setting IDENTITY?

@ellisonbg

ellisonbg Sep 6, 2011

Member

Why are these lines commented out? Should we not be setting IDENTITY?

This comment has been minimized.

@minrk

minrk Sep 6, 2011

Member

Possible oversight (though session.session is unicode, and not appropriate for IDENTITY)

@minrk

minrk Sep 6, 2011

Member

Possible oversight (though session.session is unicode, and not appropriate for IDENTITY)

This comment has been minimized.

@ellisonbg

ellisonbg Sep 6, 2011

Member

Should we just delete the lines rather than commenting out then?

On Tue, Sep 6, 2011 at 2:18 PM, Min RK
reply@reply.github.com
wrote:

@@ -187,8 +187,8 @@ class ShellSocketChannel(ZMQSocketChannel):

     def run(self):
         """The thread's main activity.  Call start() instead."""

  •        self.socket = self.context.socket(zmq.XREQ)
  •        self.socket.setsockopt(zmq.IDENTITY, self.session.session)
  •        self.socket = self.context.socket(zmq.DEALER)
  •        # self.socket.setsockopt(zmq.IDENTITY, self.session.session)

Possible oversight (though session.session is unicode, and not appropriate for IDENTITY)

Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/758/files#r114967

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg

ellisonbg Sep 6, 2011

Member

Should we just delete the lines rather than commenting out then?

On Tue, Sep 6, 2011 at 2:18 PM, Min RK
reply@reply.github.com
wrote:

@@ -187,8 +187,8 @@ class ShellSocketChannel(ZMQSocketChannel):

     def run(self):
         """The thread's main activity.  Call start() instead."""

  •        self.socket = self.context.socket(zmq.XREQ)
  •        self.socket.setsockopt(zmq.IDENTITY, self.session.session)
  •        self.socket = self.context.socket(zmq.DEALER)
  •        # self.socket.setsockopt(zmq.IDENTITY, self.session.session)

Possible oversight (though session.session is unicode, and not appropriate for IDENTITY)

Reply to this email directly or view it on GitHub:
https://github.com/ipython/ipython/pull/758/files#r114967

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

This comment has been minimized.

@minrk

minrk Sep 6, 2011

Member

I was remembering incorrectly. I commented out the IDENTITY lines to test with libzmq-4.0, which has removed IDENTITY altogether. Session.session is not unicode until PR #663 is merged, which makes the appropriate change to Session.bsession. I have rolled back these lines with an amended commit to prevent conflicts. This PR now does not touch those lines.

@minrk

minrk Sep 6, 2011

Member

I was remembering incorrectly. I commented out the IDENTITY lines to test with libzmq-4.0, which has removed IDENTITY altogether. Session.session is not unicode until PR #663 is merged, which makes the appropriate change to Session.bsession. I have rolled back these lines with an amended commit to prevent conflicts. This PR now does not touch those lines.

@ellisonbg

View changes

IPython/zmq/session.py
+ log.error('%s', msg_list)
+ log.error('%s, %s, %s', msg_list[0], DELIM, msg_list[0] == DELIM)
+ raise
+ # print msg_list.index(DELIM)

This comment has been minimized.

@ellisonbg

ellisonbg Sep 6, 2011

Member

Let's remove the commented lines (time.sleep and print msg_list...). Also, do we want Session to depend on Application?

@ellisonbg

ellisonbg Sep 6, 2011

Member

Let's remove the commented lines (time.sleep and print msg_list...). Also, do we want Session to depend on Application?

This comment has been minimized.

@minrk

minrk Sep 6, 2011

Member

Ah, now I get it. That whole thing is a debug block I was using, and should be removed (it has been). IPython.zmq.session is not changed at all now.

@minrk

minrk Sep 6, 2011

Member

Ah, now I get it. That whole thing is a debug block I was using, and should be removed (it has been). IPython.zmq.session is not changed at all now.

use ROUTER/DEALER socket names instead of XREP/XREQ
This is principally a big find/replace, but also
adjusts the import-check for pyzmq/zmq versions in IPython.zmq and IPython.parallel.

XREP/XREQ are aliases for ROUTER/DEALER in 0MQ 2.x.  These sockets continue to exist in 3.0 under the ROUTER/DEALER name only.  The XREP/XREQ protocols change some in 3.0, and won't work properly with current IPython.

It is likely that once 3.0 is stable (and pyzmq supports it better), we will want to move some sockets back to the *new* XREP/XREQ, but this PR should make IPython safe through libzmq-3.x.

@minrk minrk merged commit 7bde2f3 into ipython:master Sep 6, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment