Multipart Message Support #9

Merged
merged 1 commit into from Jun 10, 2011

Conversation

Projects
None yet
4 participants
Owner

ianbarber commented Jun 5, 2011

Right now, as far as I can tell there isn't any way of detecing multipart messages in active mode - this is problematic for using XREP sockets in particular. If there is a way of doing this at the moment, please let me know (I absolutely could have missed it).

Use case is a simple queue: req -> [xrep -> xreq] -> rep

======= The rest of this message is the original proposal, which is no longer correct! =======
Assuming I haven't missed something, I've attached a patch which is really my sketch of an idea on how to process them. The patch does a RCVMORE based do while loop, and puts all parts into a list.

Right now, this means an API change, as even individual messages are returned in a list, but I would guess it's probably better to be consistent on this part. I would say that it's likely to want a version of send that accepts a list of binaries - happy to add that as well if this seems like a good way to go (would need doc updates as well, so I don't expect this patch to be merged, more a starting point for discussion).

Contributor

gar1t commented Jun 8, 2011

I'm filling in the missing Erlang examples from the guide (see https://github.com/gar1t/zguide) and bumped into this exact issue.

The rrbroker example highlights the problem. I've stubbed out the example code here:

https://github.com/gar1t/zguide/blob/master/examples/Erlang/rrbroker.es

This escript has various "loop" implementations. In particular, loop_parts/2 shows your proposed API. I added another possible approach with loop_tagged_msg/2, which tags messages in active mode with 'more' or 'last'.

Assuming I haven't missed anything with the tagging approach (e.g. odd edge cases where messages could be received in a different order), that approach feels closer to the ZeroMQ tao of handling multiparts. It also skirts the memory problem of constructing the list in the NIF (e.g. I have an unpulled patch for the first erlzmq that uses a vector to build the list). In any case, just an idea.

To save readers some time, here's the parts-as-list API from the example:

loop_parts(Frontend, Backend) ->
    receive
        {zmq, Frontend, Parts} -> send_parts(Backend, Parts);
        {zmq, Backend, Parts} -> send_parts(Frontend, Parts)
    end,
    loop_parts(Frontend, Backend).

send_parts(Socket, [Part]) ->
    erlzmq:send(Socket, Msg);
send_parts(Socket, [Part|Rest]) ->
    erlzmq:send(Socket, Part, [sndmore]),
    send_parts(Socket, Rest).

I agree that "send_parts" should really be erlzmq:send(Socket, Parts) -- or maybe erlzmq:sendmulti(Socket, Parts) to avoid confusion with the zmq API.

Here's the tagged-message API:

loop_tagged_msg(Frontend, Backend) ->
    receive
        {zmq, Frontend, {more, Msg}} ->
            erlzmq:send(Backend, Msg, [sndmore]);
        {zmq, Frontend, {last, Msg}} ->
            erlzmq:send(Backend, Msg);
        {zmq, Backend, {more, Msg}} ->
            erlzmq:send(Frontend, Msg, [sndmore]);                              
        {zmq, Backend, {last, Msg}} ->
            erlzmq:send(Frontend, Msg)
    end,
    loop_tagges_msg(Frontend, Backend).

This is not as elegant as receiving a list, but it stays closer to the zmq API.

Though admittedly I'm not a fan of the sndmore/rcvmore boilerplate that shows up all over the place in the examples.

Contributor

gar1t commented Jun 8, 2011

Also, looking at loop_broke/2 in rrbroker.es, I'd say that erlzmq:getsockopt(S, rcvmore) should be an error with active sockets. It's easy to write that code (esp if you're following anyone else's examples) thinking it should work.

Owner

ianbarber commented Jun 9, 2011

OK, I've added a new commit that contains the format we talked about at Erlang Factory of flagging the message on recv. Gar1t - good point on the push, your implementation is fairly sane, and not massively different to what was in zmsg in the earlier versions of the guide. I think the flagging of multipart inbound is a limitation if we don't have it, but everything else can be built in userland - whether we add it to the library or not is a good discussion though.

Owner

ianbarber commented Jun 9, 2011

Here's an example of what calling code will look like with this change:

-module(mptest).
-export([run/0]).

run() ->
        {ok, Context} = erlzmq:context(),
        {ok, Sock} = erlzmq:socket(Context, [pull, {active, true}]),
        ok = erlzmq:bind(Sock, "tcp://*:8989"),
        loop().

loop() ->
        receive
                {zmq, Sock, Data, [rcvmore|[]]} ->
                        recvall([Data], Sock);
                {zmq, _Sock, Data, _Flags} ->
                        io:format("Single: ~s ~n", [Data]),
                        loop()
        end.

recvall(Current, Sock) ->
        receive
                {zmq, Sock, Data, [rcvmore|[]]} ->
                        recvall(Current ++ [Data], Sock);
                {zmq, Sock, Data, _Flags} ->
                        io:format("Multi: ~p~n", [Current ++ [Data]]),
                        loop()
        end.
Contributor

gar1t commented Jun 9, 2011

Heh, funny -- my last|more tagging scheme was annoying me and I woke up this morning thinking of the same proplist scheme you have there.

I have that change teed up in a fork that I'm testing the Erlang examples on. I'll post here shortly.

Contributor

gar1t commented Jun 9, 2011

For my two-cents, that "flags" API feels just right.

gar1t/erlzmq2@123f772

The rrbroker example (tested with rrclient and rrserver, works well) looks like this:

https://github.com/gar1t/zguide/blob/master/examples/Erlang/rrbroker.es

Contributor

gar1t commented Jun 9, 2011

Eh, sorry -- I didn't notice your commit :) I'll pull.

Contributor

gar1t commented Jun 9, 2011

I like everything here except the flags list on erlzmq:recv. I think breaking with the C API here is unnecessary and makes it awkward to use. Since you can always, and correctly, explicitly check for rcvmore on the socket, I'd leave that as it is.

On the other hand, receiving a 'zmq' tagged message in active mode is Erlang specific, so I think it's fine to augment that interface without feeling a need to make things symmetric with the passive API. Since it's impossible to correctly read the rcvmore flag at the time an Erlang message is received, I think the only option there is to provide the flag. The property list as a 4th element I think is the right interface.

Btw, this could be extended to other flags that can't accurately be read from the socket at the time the Erlang message is received. I'm not sure if there are any others like rcvmore though.

yrashk added a commit that referenced this pull request Jun 10, 2011

Merge pull request #9 from ianbarber/master
Multipart Message Support

@yrashk yrashk merged commit 0ec0347 into zeromq:master Jun 10, 2011

Member

yrashk commented Jun 10, 2011

Reverting this pull request merge.

Consistently getting these errors in tests:

Assertion failed: nbytes == sizeof (command_t) (mailbox.cpp:194)
Invalid argument
rc == 0 (mailbox.cpp:178)
Assertion failed: new_sndbuf > old_sndbuf (mailbox.cpp:183)
Assertion failed: nbytes == sizeof (command_t) (mailbox.cpp:194)
Invalid argument
rc == 0 (mailbox.cpp:178)
Contributor

evax commented Jun 10, 2011

Yurii, works for me here on Linux.
Wouldn't it be that you didn't setup your new macbook for zeromq, as mentioned on the recent thread with Kevin?

Owner

ianbarber commented Jun 10, 2011

Hmm, that error sounds like something other than multipart - did evax's suggestion help?

Owner

ianbarber commented Jun 10, 2011

Gar1t - i see where you're coming from, I had the same reaction initially, but in discussion we came to the conclusion it was probably the right move to have the flags - I think that those of use that do guide translations look to minimize the differences between the bindings more than strictly necessary. Perhaps we could make this optional or similar.

Member

yrashk commented Jun 10, 2011

Sorry guys, it was my bad. All good.

Contributor

gar1t commented Jun 10, 2011

Ian, in your discussions on the flags, what was the gist of the argument that swayed the debate (in the context of recv, not in the 'zmq' messages sent in active mode)?

Looking over the examples I'm filling in (https://github.com/gar1t/zguide/tree/master/examples/Erlang), I'm not sure that API change has much value. Of the 34 calls to recv, there are three checks for rcvmore.

Even that was used more often, it doesn't really clean much up. Here are the different uses (as you well know, just getting them on paper):

    {ok, Msg, Flags} = erlzmq:recv(Socket),
    case proplist:get_bool(rcvmore, Flags) of
        true -> handle_more();
        false -> nomore()
    end
    {ok, Msg} = erlzmq:recv(Socket),
    case erlzmq:getsockopt(Socket, rcvmore) of
        {ok, 1} -> handle_more();
        {ok, 0} -> nomore()
    end

I'm not seeing the payoff here that would justify these costs:

  • Big break in the API, which (at least based on what I'm seeing in the examples) would go unused most of the time
  • Cost of looking up that flag on every call - this might be small, but it's there (and forced, ick)
  • Potential source of fugliness and bugs (if you're tempted to match directly on the list)

On the last point, for example, "[rcvmore|[]]" as you have above (did you possibly mean "[rcvmore|_]"?) is an unorthodox (and generally dangerous) pattern match on a proplist. If that element is actually a list of flags, rcvmore should be read using lists:keyfind or proplists:get_value, in which case the developer is just as well off querying the socket. If the API requires that list to be strictly ordered, it probably wants to be a tuple or record.

Okay, almost done... one more point :)

I personally think that the language variants should stay as close to the zmq API as possible. One of the appealing features of zmq is that it's drop dead simple and, in theory, one can easily leverage it just about anywhere. That's a big synergy. But it gets diluted when languages stray from the core API. I think those decisions should have a high standard to meet.

As you mentioned, perhaps this could be addressed by having another function:

erlzmq:recvflags(Socket)

IMO this is YAGNI, but at least it stays out of the way of the core API.

Okay, officially done :)

Owner

ianbarber commented Jun 10, 2011

The discussion was basically on maintaining similarity to the active mode call. To be honest, I dunno - I am happy to submit a patch to put recv back to a 2-tuple but I'd like to hear yrashk or evax's opinion on it. It's about a 2 minute change, so I'm fine with that!

Owner

ianbarber commented Jun 10, 2011

OK, separate issue opened: #12

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