why socket_t::send accepts a ref of message_t, but recv accepts pointer? about cppzmq

[grefab]

It's quite common (and AFAIK good practice) to have a pointer type as parameter (not a reference) when you know this parameter's content may be changed by the function. If the parameter's content will not change, usually a const reference is used.

More explanation can be found for example here: https://google.github.io/styleguide/cppguide.html#Reference_Arguments

This conventions would explain the pointer in recv's signature. However, I agree that send should have a const ref. This, though, may be impossible (without a const_cast) if the C interface (i.e. zmq_msg_send, https://github.com/zeromq/cppzmq/blob/master/zmq.hpp#L604) does not handle constness, I haven't checked that.

[derekxgl]

btw, google does not use exceptions in its cppguide :)

[grefab]

Okay, I've checked. The C interface needs a zmq_msg_t*, which conflicts if
send (message_t &msg_, int flags_ = 0)
becomes
send (const message_t &msg_, int flags_ = 0)

However, this can be fixed by const_casting:

before:

inline bool send(message_t& msg_, int flags_ = 0)
{
    int nbytes = zmq_msg_send(&(msg_.msg), ptr, flags_);
    ...
}

after:

inline bool send(const message_t& msg_, int flags_ = 0)
{
    int nbytes = zmq_msg_send(const_cast<zmq_msg_t*>(&(msg_.msg)), ptr, flags_);
    ...
}

I personally find const_casts ugly, but in order to have cppzmq provide a clean interface this may be an appropriate change. Any comments?

[grefab]

Digging deeper: No need for (another) cost_cast. message_t's data() has that already. So I propose send() becomes this:

inline bool send(const message_t& msg_, int flags_ = 0)
{
    int nbytes = zmq_msg_send((zmq_msg_t*) msg_.data(), ptr, flags_);
    if (nbytes >= 0)
        return true;
    if (zmq_errno() == EAGAIN)
        return false;
    throw error_t();
}

[derekxgl]

I'm new to zeromq and its cpp api. Obviously it's wrong/dangerous to accept a const ref but const away to modify it internally.

[grefab]

Yeah, it is. I assumed that sending a message does not modify it, but I am wrong. From the guide:

The zmq_msg_t structure passed to zmq_msg_send() is nullified during the call. If you want to send the same message to multiple sockets you have to copy it using (e.g. using zmq_msg_copy()).

(See http://api.zeromq.org/4-0:zmq-msg-send)

So the const_cast is actually a bad idea. In that case, I'd like to get rid of the reference at all and just keep the pointer signature of send(). That, however, will break backwards compatibility in order to stick to a non-standard convention, so it's not a good idea either.

[derekxgl]

Certainly you can overload socket_t::send with a new one accepting a pointer to message_t and leave the one with message_t ref unchanged to keep it backward compatible.

[szmcdull]

There is a new zmq_send_const API in ZMQ 4.1.4. Please add support for this.

[aytekinar]

I have stumbled upon this thread when I also thought about why they had different signatures. Could you please clarify the need to require send and recv methods to have different signatures, @grefab ? I am not an expert in C++, but yet again, I have read some articles such as this which has fairly valid points against google style guide.

in my point of view, they should both have non-const references. having recv receive message_t as a reference will help have a symmetric set of definitions in both recv and send, except for the corner-case of sending messages based on iterators. In that way, we would be able to use rvalue references for the recv (I am not sure if that helps, at all, but at least we can have a consistent API), too. Another advantage of having references is, in my opinion, to have valid objects, in the sense that, we cannot have uninitialized object references whereas we can just pass uninitialized object pointers to those functions.

what do you think @grefab and @derekxgl ?

[joncage]

I also fell foul of this: http://stackoverflow.com/questions/41893295/why-does-zeromq-req-rep-give-me-an-empty-response-between-c-and-python

[aytekinar]

@joncage, I think your problem is mostly related to confusing/mixing the C API with that of C++.

If you are going to use C++ API (this API), you should try to avoid pointers --- they are not encouraged in modern C++ due to a lot of reasons (unless you need some polymorphism --- and, in this case, you are not using polymorphism on the ZMQ C++ API).

Then, this API already provides you with a handful of data types, i.e., templated classes, that you can use to get rid of the clutter in your C++ application. Specifically, for the very simple example you have provided in SO, you can benefit from iterators and constructors defined for the std::string class. Please check the modified version below:

#include <iostream>
#include <string>

#include <zmq.hpp>

int main(int argc, char* argv[]) {
  constexpr unsigned int port { 5563                                            };
  zmq::context_t  context     {                                                 };
  zmq::socket_t   publisher   { context, zmq::socket_type::rep                  };
  std::string     bindDest    { std::string("tcp://*:") + std::to_string(port)  };
  zmq::message_t  request     {                                                 };

  publisher.bind(bindDest);

  bool notInterrupted = true;
  while (notInterrupted)
  {
      // Wait for a new request to act upon.
      publisher.recv(&request);

      // Turn the incoming message into a string
      std::string requestStr      { (char *) request.data()   };
      std::cout << "Got request: " << requestStr << std::endl;

      // Call the delegate to get a response we can pass back.
      std::string responseString  {  requestStr + " response" };
      std::cout << "Responding with: " << responseString << std::endl;

      // Turn the response string into a message.
      zmq::message_t responseMsg  { responseString.cbegin(),
                                    responseString.cend()     };

      // Pass back our response.
      publisher.send(responseMsg);
  }

  return 0;
}

As you can realize, getting rid of the pointers and C-style character arrays in your code help you write cleaner (easily readable) code with (hopefully) no memory leakage.

Even then, you have the above-mentioned problems due to the inconsistent implementation of the recv and send member functions of socket_t type objects. For me, and for some other people here, they should both have either references to the objects or pointers. However, the API does not allow for it. This discussion is all about it. I hope this makes it more clear for you.

I apologize if I have got you bored by (maybe) explaining something that is already obvious for you (I do not know your experience in C++, and I cannot consider myself as an expert, either).

[joncage]

I must admit I've lived on the edge of the embedded world for a while and I sometimes miss the niceties available with the std classes.

Unfortunately we're limited to the Visual Studio 2008 tool chain which limits some of the niceties (no std::to_string for us I'm afraid). There are a number of improvements in your example that we can however use so thanks very much for that.

One day we'll join the modern world...!

Your suggestion to unify the interface with references would certainly have helped me in avoiding that SO post.

[aytekinar]

Actually I have solved my issue by simply adding

inline bool recv (message_t &msg_, int flags_ = 0)
{ return recv(&msg_, flags_); }

under the definition of recv for the pointer type. I have added this repository as a submodule to my project, too. As a result, I can always fetch the updates from the upstream, and yet, keep my introduced changes on top.

Most likely, you have already thought about this solution, but I still wanted to let you know. It might not be the most efficient (as the object code will increase in size, since the above method is not templated), and thus, this might not be wanted in your embedded solution, either, though.

[robwiss]

I have a further issue here which is that with a send flag included socket_t::send actually will accept a pointer to a message. If someone is a little careless and makes their send calls like their recv calls, they will end up experiencing runtime errors that ought to have been caught at compile time. Here's some example code to demonstrate what happened to me.

void f(zmq::socket_t& skt, std::vector<zmq::message_t> envelope, const std::string& payload) {
  for (auto& m : envelope) {
    skt.send(&m, ZMQ_SNDMORE);
  }
  
  skt.send(std::begin(payload), std::end(payload));
}

Can you spot the problem? skt.send(&m, ZMQ_SNDMORE) calls the raw pointer overload:

inline size_t send(const void *buf_, size_t len_, int flags_ = 0)

I propose socket_t::send be changed to make this impossible. One way to do this is to create an enum type to represent the flags then change the flags type everywhere to accept only this type.

namespace zmq {
enum class send_flags : int {
  none = 0x0,
  dontwait = 0x1,
  sndmore = 0x2
};

send_flags operator|(const send_flags& lhs, const send_flags& rhs) {
  return static_cast<send_flags>(
      static_cast<int>(lhs) | static_cast<int>(rhs));
}

class socket_t {
...
inline size_t send(const void *buf_, size_t len_, send_flags flags_ = send_flags::none);
inline bool send(message_t &msg_, send_flags flags_ = send_flags::none);
template<typename T> bool send(T first, T last, send_flags flags_ = send_flags::none);
inline bool send(message_t &&msg_, send_flags flags_ = send_flags::none) { return send(msg_, flags_); }
...
};
}

With this change, my error would have been impossible. I would've used send_flags::sndmore instead of ZMQ_SNDMORE as my second argument, and this would've caused a compiler error since the enum can't be coerced to a size_t without an explicit cast.

Another improvement would be to drop the socket_t::send raw pointer overload in favor of one that accepts the gsl::span type, which is a view for a contiguous area of memory. https://github.com/Microsoft/GSL/blob/master/include/gsl/span. This, too, would have prevented my error.

[kurdybacha]

yeah, agree that this API is not great. We have already have socket_type so send_flags can follow the same pattern. Just instead changing existing functions which is breaking change we would need to add new overloads with send_flags and deprecate (ZMQ_DEPRECATED) problematic ones and remove them at some stage in the future.

…

On Tue, Mar 26, 2019 at 6:13 PM robwiss ***@***.***> wrote: I have a further issue here which is that with a send flag included socket_t::send actually will accept a pointer to a message. If someone is a little careless and makes their send calls like their recv calls, they will end up experiencing runtime errors that ought to have been caught at compile time. Here's some example code to demonstrate what happened to me. void f(zmq::socket_t& skt, std::vector<zmq::message_t> envelope, const std::string& payload) { for (auto& m : envelope) { skt.send(&m, ZMQ_SNDMORE); // calls socket_t::send(const void *buf_, size_t len_, int flags_ = 0) } skt.send(std::begin(payload), std::end(payload)); } Can you spot the problem? skt.send(&m, ZMQ_SNDMORE) calls the raw pointer overload: inline size_t send(const void *buf_, size_t len_, int flags_ = 0) I propose socket_t::send be changed to make this impossible. One way to do this is to create an enum type to represent the flags then change the flags type everywhere to accept only this type. namespace zmq { enum class send_flags { none = 0x0, dontwait = 0x1, sndmore = 0x2 }; class socket_t { ... inline size_t send(const void *buf_, size_t len_, send_flags flags_ = send_flags::none); inline bool send(message_t &msg_, send_flags flags_ = send_flags::none); template<typename T> bool send(T first, T last, send_flags flags_ = send_flags::none); inline bool send(message_t &&msg_, send_flags flags_ = send_flags::none) { return send(msg_, flags_); } ... }; } With this change, my error would have been impossible. I would've used send_flags::sndmore instead of ZMQ_SNDMORE as my second argument, and this would've caused a compiler error since the enum can't be coerced to a size_t without an explicit cast. Another improvement would be to drop the socket_t::send raw pointer overload in favor of one that accepts the gsl::span type, which is a view for a contiguous area of memory. https://github.com/Microsoft/GSL/blob/master/include/gsl/span. This, too, would have prevented my error. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADROQGraa2DBUQwdzyEd16fKfReWV_pBks5vamNDgaJpZM4HIGFf> .

[sigiesec]

Agree with regard to the type safety of the flags parameter.

Regarding gsl, I am hesitant to adding a dependency to that, as it might make the integration of cppzmq harder.

[kurdybacha]

Regarding gsl, I am hesitant to adding a dependency to that, as it might

make the integration of cppzmq harder. +1

…

On Wed, Mar 27, 2019 at 11:36 AM Simon Giesecke ***@***.***> wrote: Agree with regard to the type safety of the flags parameter. Regarding gsl, I am hesitant to adding a dependency to that, as it might make the integration of cppzmq harder. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADROQJ45AB4YMQnBjLGpVwIMsSSGuO9bks5va1ewgaJpZM4HIGFf> .

[robwiss]

yeah, agree that this API is not great. We have already have socket_type so send_flags can follow the same pattern. Just instead changing existing functions which is breaking change we would need to add new overloads with send_flags and deprecate (ZMQ_DEPRECATED) problematic ones and remove them at some stage in the future.

When adding send_flags would it be important for the send_flags type to support any bitwise operators other than operator|?

If send_flags is added should enum types for the other sets of flags be added for consistency?

[sigiesec]

Maybe that depends. The send flags are only used as input parameters, so operator | is probably sufficient for most uses. Other flags that are also results should have operator & at least.
It would be great to have a consistent mapping of all flags, so if you have the time to do that, it would be great :)