search

zeromq / zproto

A protocol framework for ZeroMQ
MIT License
237 stars 100 forks source link

Problem: zproto client works with messages with msg type only #394

Closed vyskocilm closed 6 years ago

vyskocilm commented 6 years ago

Solution: adapt term virtual for recv methods, so virtual="0" will change prototype in a following way

// virtual == 1
zmsg_t * foo_client_recv (foo_client_t *self);
// virtual == 0
int foo_client_recv (foo_client_t *self);

This is a bit similar to virtual="0" in zproto_codec_c

// virtual == 1
zmsg_t * foo_msg_recv (void *socket);
// virtual == 0
int foo_msg_recv (foo_msg_t *self, void *socket);

Defaults for codec_c and client_c are different to maintain backward compatibility.

somdoron commented 5 years ago

@vyskocilm I think this pull request leak some memory.

The properties are not destroyed as part of the class destructor, maybe the allocated property should be set to 1? here: https://github.com/zeromq/zproto/blob/master/src/zproto_client_c.gsl#L271

Another issue, you must free the properties before calling recv again.

I can work on a fix, I thought maybe you want to do it.

vyskocilm commented 5 years ago

Hi Doron,

you're right about missing destructor, that's serious problem.

However I am not sure about recv - @hintjens told me back then, that zproto_client API is designed around minimizing memory allocations, so there "might" be garbage left from previous call. Especially when you recv different types of messages. I can't say what is your use-case, however it wasn't a problem in our code. One simply should not read fields which does not belong to the message type you're processing.

Can't say when I will be able to fix that ...

On Sun, Jan 13, 2019 at 8:14 PM Doron Somech notifications@github.com wrote:

@vyskocilm https://github.com/vyskocilm I think this pull request leak some memory.

The properties are not destroyed as part of the class destructor, maybe the allocated property should be set to 1? here: https://github.com/zeromq/zproto/blob/master/src/zproto_client_c.gsl#L271

Another issue, you must free the properties before calling recv again.

I can work on a fix, I thought maybe you want to do it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zeromq/zproto/pull/394#issuecomment-453857091, or mute the thread https://github.com/notifications/unsubscribe-auth/AIa-1LSfyNIePoMTQ8aepvBjKyfTrd-iks5vC4WHgaJpZM4RUPQX .

-- best regards Michal Vyskocil

somdoron commented 5 years ago

that not the case for bsend. those properties must be freed before calling bsend again. same as the command. I will fix it. Thanks

somdoron commented 5 years ago

Actually the bug was there before the pull request, I think.