api changes / refactoring about em-zeromq HOT 27 CLOSED

RE: replace ffi-rzmq with "official" zmq

I am likely biased because I'm the ffi-rzmq author, but the zmq gem is no more official than the ffi-rzmq gem. As a maintainer of the zeromq project, I will eventually just move the ffi-rzmq gem under that github "organization" but I don't plan to do that until I stabilize the API with a 1.0 release.

RE: define facades for context & socket

Good ideas, both. The low-level gems (either zmq or ffi-rzmq) are simple ports of the C API without any (or many) ruby idioms. Wrapping them up inside another class to provide those idioms would be a good idea.

FYI, this library (em-zeromq) is mostly unmaintained. Neither @andrewvc or myself have the time or inclination to maintain it. If anyone wants to volunteer to take over as maintainer, I'm sure we could come to an arrangement.

from em-zeromq.

Thanks Chuck, I went by http://www.zeromq.org/bindings:ruby

I certainly don't mind ffi-rzmq if it offers specific advantages over rzmq. I also don't mind maintaining this gem, I might potentially be using this a lot in my work.

from em-zeromq.

Hey guys, just fyi, @schmurfy is now the maintainer of this project.

from em-zeromq.

yeah what i initially thought. i raised this ticket on his suggestion.

from em-zeromq.

Replace ffi-rzmq with officially supported zmq
ffi bindings have a strong advantage over C extension which is to not lock us on a specific ruby implementation, I will currently use the gem with MRI 1.9.3 but I don't want to prevent it from running under Rubinius or JRuby.
As chuckremes pointed the zmq gem is no more official than ffi-rzmq and honestly is it really important ? I tend to mostly disregard those details as long as it does what I want it to and chuckremes is doing a really good job with ffi-rzmq to ensure the gem works with the latest zeromq releases.

Define façades for ZMQ::Context, ZMQ::Socket
I may already have used this but I am not familiar with the "facade" term, could you explain or even show me an example of what you mean by that ?

EM::ZeroMQ::Connection should inherit from EM::Connection
This is already true in the current codebase :)

Each class should only be responsible for one thing only, akin to the single responsibility principle
I mostly agree on this.

I checked your fork and I like your interface but I have some remarks/comments:

• why not merge the socket and connection clases ? connect/bind take a handler as parameter but judging by the code they will all get called for any incoming packet anyway since the zeromq socket is the same (I am not even sure how eventmachine reacts if you attach the same descriptor multiple times).

What about the following ? (I just checked, zeromq can return us the file descriptor just after the socket creation so we can do the attach when the socket is created by the context class and just pass the connect/bind calls to ffi-rzmq socket after)

socket = ctx.socket(ZMQ::PULL, EMTestPullHandler.new)
socket.connect('tcp://127.0.0.1:2091')
socket.connect('ipc:///tmp/a')
socket.connect('inproc://simple_test')

• the block syntax for the socket call may be usefull but I prefer to stay away of instance_eval used like this, I consider it more confusing than helpful since you it pulls the user away for his current context:

host = 43

ctx.socket(ZMQ::PUSH) do
  # error: host is undefined
  connect(host)
end


# versus
host = 43

ctx.socket(ZMQ::PUSH) do |s|
  s.connect(host)
end

from em-zeromq.

Define façades for ZMQ::Context, ZMQ::Socket

I think its priority for ruby community and ZMQ. And it will be automatically EMified with em-sync adapter https://github.com/prepor/em-synchrony/blob/master/lib/em-synchrony/zmq.rb ;)

from em-zeromq.

ffi-rzmq vs zmq

I almost only ever use MRI at home and work. But that might change, so I agree with that point that it can help with JRuby. I think zmq as is works fine with Rubinius head.

In addition to this, I think use of rb_thread_blocking_region in zmq is more elegant but that's just me.

façades

They are essentially adapters^[1] that hide library specifics from the user. You already have some of it in the code, but have the socket and connection classes combined that deviates from the singular responsibility principle mentioned earlier.

[1] http://en.wikipedia.org/wiki/Facade_pattern

block syntax and instance eval

agree, context.socket(...) can pass the instance to the block if given, it is definitely a better approach and less magic.

example

I think it would be more obvious as below. It would emulate the way connections are defined in EM and would offer less surprises to somebody already familiar with it.

class MyHandler < EM::ZeroMQ::Connection
end

socket = ctx.socket(ZMQ::PULL)
socket.connect('tcp://127.0.0.1:2091', MyHandler, *args)
socket.connect('ipc:///tmp/a', MyHandler, *args)
socket.connect('inproc://simple_test', MyHandler, *args)

keeping socket and connection classes separate

In addition each class doing one thing and one thing well, it also allows us to define a leaner connection class that only deals with notifications and callbacks. By keeping socket class separate, you can expose or encapsulate socket related api in a more cleaner fashion.

zeromq already does a great job with its api, I don't think we need to reinvent anything - just write some thing wrappers around it to get it going in eventmachine.

EM and multi watch or attach

"I am not even sure how eventmachine reacts if you attach the same descriptor multiple times."

EM will fail assertion and exit afaik

from em-zeromq.

@prepor good suggestion, if we can make Socket#recv return a deferrable instance and that would make it easier to use em-synchrony.

from em-zeromq.

@schmurfy I'm thinking of slowly gutting my hack to incorporate the ideas here and while I'm at it have a crack at supporting both zeromq 2.1 and 3.1.

I can switch to ffi-rzmq while I'm at it, but ffi-rzmq would need to be updated as well to support the changes in 3.1

from em-zeromq.

I think if we all agree, we can keep working on this in a branch and then once the api is stable and satisfactory merge it to master.

from em-zeromq.

ffi-rzmq vs zmq
ffi-rzmq already support all zeromq versions including 3.1, I don't see any reason to switch to the zmq gem (which does not seems to support 3.1).

keeping socket and connection classes separate
I really don't get your argument on this one, you refer to the C api but it has no "connection" concept only contexts and sockets, how adding an object that does not exists in the original API helps it be closer to the original API ?

How the singular responsibility principle apply there ? The Socket object would handle one zeromq socket and nothing else, it does have a single responsibility for me.

There a major problem with your example code: it does not work, you cannot have more than one handler for one socket,
I tried this with your fork:

require 'rubygems'
require File.expand_path('../lib/em-zeromq', __FILE__)

class Handler1 < EM::ZeroMQ::Connection
  def on_readable(msg)
    puts "handler1: #{msg}"
  end
end

CTX = EM::ZeroMQ::Context.new(3)


EM.run do  
  socket2 = CTX.socket(ZMQ::REQ)
  req1 = socket2.connect('tcp://127.0.0.1:5555', Handler1)
  req2 = socket2.connect('tcp://127.0.0.1:5556', Handler1)
end

And it just crash which is probably the result of the multiple attach you are doing as you mentioned, the only way I see this work is like that:

EM.run do  
  socket = CTX.socket(ZMQ::REQ, Handler1)
  socket.connect('tcp://127.0.0.1:5555')
  socket.connect('tcp://127.0.0.1:5556')

  # ...
  socket.send_msg(...)
end

When you do 4, 5 or even 20 connect/bind calls with different endpoints on the same socket in zeromq you do not have 4, 5 or 20 connections but only one virtual connection which is the socket itself, when you write or read you do so on the socket too and what happens on the system sockets below depends on other parameters like the socket type.
You never access the real unix sockets directly which is the whole point of the zeromq library: hiding the low level details from the developers.

from em-zeromq.

_ffi-rzmq_

I was talking about http://api.zeromq.org/3-1:zmq-getmsgopt - I'm not even sure we need it if we're not aiming at a complete implementation.

_keeping socket and connection classes separate_

Think about the Socket class in terms of managing the raw zmq_socket and the Connection class being responsible for handling eventmachine related events. For example, if for some reason the handler wants to tear down the connection on reaching a certain state, it can do so because the handler is the class responsible for the connection (well this is not perfect in cases where a socket is connected to multiple endpoints)

_multiple endpoints_

Yes there is no way to do it at present, but its easy enough to do. All I had to do was make Connection#socket public.

EM.run do
  class Handler < EM::ZeroMQ::Connection
    def on_readable message
      puts message
    end 
  end 

  context = EM::ZeroMQ::Context.new(2)
  pub1    = context.socket(ZMQ::PUB).bind('tcp://*:5555')
  pub2    = context.socket(ZMQ::PUB).bind('tcp://*:5556')
  sub     = context.socket(ZMQ::SUB) do |socket|
    socket.subscribe('')
    connection = socket.connect('tcp://*:5555', Handler)
    connection.socket.zmq_socket.connect('tcp://*:5556')
  end 

  EM.add_periodic_timer(1) do
    pub1.send('hello 1')
    pub2.send('hello 2')
  end 
end

We could think of a more saner api for this pattern where a single handler handles multiple endpoints.

from em-zeromq.

In light of using multiple endpoints your example above makes more sense, passing a handler to a socket creation api is not the best approach I can think of at this time but it is surely something that will emphasise the fact that you can have only only handler per socket.

context.socket(ZMQ::SUB, Handler) do |socket|
  socket.subscribe('')
  socket.connect('tcp://*:5555')
  socket.connect('tcp://*:5555')
end

which will also work in a non-block form.

from em-zeromq.

I admit I never even saw http://api.zeromq.org/3-1:zmq-getmsgopt but since the function is currently useless not having it is not a real problem (we can already know if there is more parts with getsockopt). I worked with/on em-zeromq and ffi-rzmq for more than a year now I think and during this time ffi-rzmq has always had full support for the latest zeromq version (as well as the others).

The only thing against ffi bindings is the performance cost but it only occurs when "crossing" the frontier between C and Ruby, it would be a problem if you wanted to use a C library and called one of its method every millisecond in which case you would be best using a C ruby extension but in our case the cost is negligible.

    connection = socket.connect('tcp://*:5555', Handler)
    connection.socket.zmq_socket.connect('tcp://*:5556')

That's really ugly xD
Having multiple endpoints is not an edge case in zeromq but something normal and requiring the use of a hack to use a standard feature of the underlying library is bad, really bad.

For Socket/Connection separation:
If you want to close the socket (I really don't like using the connection term with zeromq since with never deal directly with them) you just call socket.close (or unbind in the current codebase) it looks pretty straightforward: I have a socket and I want to close it, no ?

If you separate the connection and the socket on purpose then you could get into weird states, what happens if you detach the Connection handler (which is effectively what you are doing if you to tear it down, you never close a descriptor owned by zeromq yourself ) but you keep the socket ? Any write on it will work as expected but you will never see any of the received packets ! (that's even worse since we now check for available packets after a write so you would get the answer for the previous write...)
That's just the opposite of the "singular responsibility principle" you started with, you take one thing and arbitrarily split it in two objects which are completely interdependent.

I am not against changes and I completely agree on implementing a Socket class because it makes sense and more than that some things don't work properly now without it (like setting hwm) but I will refrain from adding unnecessary things which could lead to debugging nightmares like finding out why a socket is not receiving any data when you saw them arrives on the network with tcpdump (if the Connection handler was detached but the socket is still used).

If you want to close the socket on specific event there is nothing preventing you to do so currently, you can do this:

require 'em-zeromq'

module ResponseHandler
  def self.on_readable(socket, messages)
    message = messages[0].copy_out_string

    if message == "CLOSE"
      puts "Closing socket upon client request..."
      socket.unbind
    else
      puts "Received message from client: #{message}"
      socket.send_msg("re: #{message}")
    end
  end
end

CTX = EM::ZeroMQ::Context.new(1)

EM.run do
  CTX.bind(ZMQ::REP, 'tcp://127.0.0.1:9000', ResponseHandler)
end

Another difference with what you propose: Here the handler is just that, a handler, you can pass an existing object or even a module and it will still works, it could even be a more complex object with a parent class ! On the projects where I use eventmachine directly not being to pass an existing object to connect is really a nuisance and lead to implementing a wrapper or some kind of delegator.

For a real example look here: https://github.com/igrigorik/em-http-request/blob/master/lib/em-http/http_connection.rb#L52 , the HttpStubConnection class is completely useless but that's the only way to bridge back to the HttpConnection object...
I just hate that that's why when I redesigned the api for em-zeromq I did it like that (I even wonder if it wasn't already like that before my api changes), you can pass anything you want as a handler and the methods will get called on it if they exists.

This behavior cannot really be compared with other eventmachine "drivers", most implement only a specific protocol and so they return a deferrable on which you set callbacks but this does not apply here, what would you do with a deferrable returned by socket.send_msg ? em-zeromq is more the foundation for other libraries/applications and closer to eventmachine than any of the driver.

from em-zeromq.

I admit I never even saw http://api.zeromq.org/3-1:zmq-getmsgopt but since the function is currently useless not having it is not a real problem (we can already know if there is more parts with getsockopt). I worked with/on em-zeromq and ffi-rzmq for more than a year now I think and during this time ffi-rzmq has always had full support for the latest zeromq version (as well as the others).

The only thing against ffi bindings is the performance cost but it only occurs when "crossing" the frontier between C and Ruby, it would be a problem if you wanted to use a C library and called one of its method every millisecond in which case you would be best using a C ruby extension but in our case the cost is negligible.

    connection = socket.connect('tcp://*:5555', Handler)
    connection.socket.zmq_socket.connect('tcp://*:5556')

That's really ugly xD
Having multiple endpoints is not an edge case in zeromq but something normal and requiring the use of a hack to use a standard feature of the underlying library is bad, really bad.

For Socket/Connection separation:
If you want to close the socket (I really don't like using the connection term with zeromq since with never deal directly with them) you just call socket.close (or unbind in the current codebase) it looks pretty straightforward: I have a socket and I want to close it, no ?

If you separate the connection and the socket on purpose then you could get into weird states, what happens if you detach the Connection handler (which is effectively what you are doing if you to tear it down, you never close a descriptor owned by zeromq yourself ) but you keep the socket ? Any write on it will work as expected but you will never see any of the received packets ! (that's even worse since we now check for available packets after a write so you would get the answer for the previous write...)
That's just the opposite of the "singular responsibility principle" you started with, you take one thing and arbitrarily split it in two objects which are completely interdependent.

I am not against changes and I completely agree on implementing a Socket class because it makes sense and more than that some things don't work properly now without it (like setting hwm) but I will refrain from adding unnecessary things which could lead to debugging nightmares like finding out why a socket is not receiving any data when you saw them arrives on the network with tcpdump (if the Connection handler was detached but the socket is still used).

If you want to close the socket on specific event there is nothing preventing you to do so currently, you can do this:

require 'em-zeromq'

module ResponseHandler
  def self.on_readable(socket, messages)
    message = messages[0].copy_out_string

    if message == "CLOSE"
      puts "Closing socket upon client request..."
      socket.unbind
    else
      puts "Received message from client: #{message}"
      socket.send_msg("re: #{message}")
    end
  end
end

CTX = EM::ZeroMQ::Context.new(1)

EM.run do
  CTX.bind(ZMQ::REP, 'tcp://127.0.0.1:9000', ResponseHandler)
end

Another difference with what you propose: Here the handler is just that, a handler, you can pass an existing object or even a module and it will still works, it could even be a more complex object with a parent class ! On the projects where I use eventmachine directly not being to pass an existing object to connect is really a nuisance and lead to implementing a wrapper or some kind of delegator.
For a real example look here: https://github.com/igrigorik/em-http-request/blob/master/lib/em-http/http_connection.rb#L52 , the HttpStubConnection class is completely useless but that's the only way to bridge back to the HttpConnection object...
I just hate that that's why when I redesigned the api for em-zeromq I did it like that (I even wonder if it wasn't already like that before my api changes), you can pass anything you want as a handler and the methods will get called on it if they exists. A simple use case would be a Client object using a zeromq socket, if you want to close it and reopen a new one you can just use your existing Client object as the handler.

This behavior cannot really be compared with other eventmachine "drivers", most implement only a specific protocol and so they return a deferrable on which you set callbacks but this does not apply here, what would you do with a deferrable returned by socket.send_msg ? em-zeromq is more the foundation for other libraries/applications and closer to eventmachine than any of the driver.

from em-zeromq.

ffi-rzmq and zmq

I think the API varies only slightly, we can support both if we want to. But I don't know if there is plan is for ongoing support for zmq into 3.1

That's really ugly xD

Yes it is :D but you missed the point I was trying to make that its not entirely impossible.

That's just the opposite of the singular responsibility principle

Not really, the sole responsibility of the Socket class is dealing the zmq socket alone. Nothing more. The Connection class has to deal with EventMachine notifications and process the events accordingly.

"If you separate the connection and the socket on purpose then you could get into weird states"

True, but that is where you expect the users to be more aware of what is happening. EventMachine users know that a call to detach will stop the notifications. I don't want to restrict the API because someone might do something stupid.

Using existing objects as handlers

Yes, that would make life easy in some cases but I would not entirely agree that it is good design. A wrapper is simple enough to setup and in most cases does not need to be wrapper at all. The handler could be the class that does any work related to incoming messages. If you need the handler instance to be aware of some existing state, just pass it as an argument like below.

This way you're using an appropriate class to handle messages instead of doing a double dispatch.

class Handler < EM::ZeroMQ::Connection
  def initialize arg1, arg2, arg3
    # do something here
  end
end

context      = EM::ZeroMQ::Context.new(1)
subscriber   = context.socket(ZMQ::SUB, Handler, arg1, arg2, arg3)

from em-zeromq.

To move forward I did the change I am ok with:

• added Socket class (which is the old Connection)
• Context class did a diet and now only has one method: socket(type, handler = nil), it accepts an optional block.

ffi-rzmq and zmq
Unless you have more arguments in favor of the zmq gem we keep ffi-rzmq, there no use supporting two gems doing the same thing if we gain nothing out of it (and we even loose the support for other zeromq versions).

Socket / Connection
Here is what I propose: fork the current codebase I just pushed, make the changes you want to and we can speak with something concrete.
I still think it will causes more problems than solution but you can try to convince me ;)

What is considered good design may change greatly depending of who you ask, I prefer to have something easy to use for everyone instead of a piece of art everyone agrees is just damn near perfection while they use another library do the same thing because it is too annoying to use or they can't use it at all.
Passing an existing object as handler did not fell from the sky, this what I do every time I use Eventmachine connect in a library and that's how I use em-zeromq in my in progress projects, that's a real use case not some mythical beast.
My problem is not about whether it is hard or easy to write the wrapper, my problem is why should we force the users of em-zeromq to write this useless wrapper every time they use it ?

I hated (and still do) so many project authors may be proud about for what could be considered small details, I won't cite any name but you may have the world greatest software if there is no one beside you able to write a config for it without crawling desperately the web then your software is just trash. I replaced many tools in my toolbox by simpler one which are really easy to use and powerful (nginx is good example, it works very well and you don't need three weeks to understand the configuration file).

from em-zeromq.

Hey @schmurfy have a squiz at https://github.com/deepfryed/em-zeromq-1/tree/master/lib.

I would still consider it work in progress, but it's fairly at the point where I'm comfortable with the changes. It has some small tweaks to the API.

1. Added a Connection class without affecting the API
2. You can pass in an object responding to on_readable/on_writable or a EM::ZeroMQ::Connection subclass (EM style) - both will work
3. I split Context.new(threads_or_context) into Context.new(threads) and Context.attach(context)

everything else is fairly self-explanatory, let me know if you have questions.

Also, what you think about using minitest/spec in the future instead of rspec ?

from em-zeromq.

I hate rspec, it does too many things I never even needed... Until now my favorite framework was bacon but minitest looks sweet and with some tweaks I could definitely switch to it !

I am currently giving minitest a test ride for another project, a protocol implementation on top of em-zeromq.

I will check your fork when I get the chance (hopefully today).

from em-zeromq.

@schmurfy ping, just checking if this is on your radar.

from em-zeromq.

Sorry, I have not forgotten you that's just that I am off for a two weeks holidays starting this week end and I had things to take care of before I go.

I will try to give it a more thorough look this evening otherwise it will have to wait until I come back.

from em-zeromq.

aah, cheers. have fun :)

from em-zeromq.

ok I am back ! So I checked your fork I have to admit I am a little disappointed because this fork awfully looks like a complete rewrite :s
Just look at the changes with the original code: deepfryed/em-zeromq-1@andrewvc:4adfa2b1c0b85e2ce87fd32efd03aed8a33aadd9...deepfryed:master

There is way more changes than what we were speaking about and there are things I don't even understand: why changing the Context constructor and add the following code ?

def self.attach context
  allocate.tap {|instance| instance.instance_variable_set(:@zmq_context, context)}
end

While I know the allocate methods exists I never saw any justified use for it and I am better off without it.

I am still not fan of using the handler provided as the "connection" object we may find a way to make us both happy but not by changing everything like that sorry.

from em-zeromq.

Yeah, it is a lot refactoring. But mostly api compatible with the old code.

• "why changing the Context constructor and add the following code ?"

I split the Context#initialize method which took either threads_or_context as parameter into Context#initialize which takes threads as a parameter and Context.attach which takes a ZMQ::Context instance.

I find methods that do one thing, more easier to understand especially in a large code base. If you don't like using a separate method. Then passing a hash to the constructor like :threads => 1 or :context => context is ok as well.

• "I am still not fan of using the handler provided as the "connection" object"

I'm not using it as a connection object. If you noticed, I used it as a connection if it was a class inheriting EM::ZeroMQ::Connection, else I wrapped it around a DefaultConnection which called the handler methods appropriately.

I know this goes against what I have stated in (1) but I don't see another way of doing it while allowing both Connection subclasses or handler objects to be passed in as argument.

I gave it a try, there can be a middle ground where you can rework my patch to what is acceptable to you. I'll leave it with you, cheers

from em-zeromq.

Hi guys

I have been working on a similar thing (based on crossroads this time) which I published today:
https://github.com/arohr/em-xs

Then I noticed "em-xs" was already used by schmurfy for the "em-zeromq port" to xs, maybe I should rename it...
But nevertheless let me know what you think about it.

Andy

from em-zeromq.

Yes I already have an em-xs library up and running on latest crossroads release, is there anything yours does mine don't ?
Feel free to propose a pull request there if you want something added in.

from em-zeromq.

Well, the API is different, and maybe cleaner.

from em-zeromq.