libuv udp client/server memory strategy

[Kevin Hendricks]

Hello Everyone,

I only have a few days experience with libuv, but done a lot of reading of uv.h, the libuv-dox, and other resources, and I've gotten both a udp client and udp echo server running with just libuv. I understand all the memory management is on the user, which is an area I'm pretty green in. I'm using the current version of the master branch.

Both programs are fairly simple: the client opens a socket for sending and recieving, and while loops calls to uv_udp_send hundreds of char * packets ("packet 1," "packet 2," etc.), individually malloc'd, to the server. The server opens a socket for receiving, has a receive callback that prints the message, copies that into a newly allocated buffer, sends it back to the same addr it got it from, and frees() the received buf->base. The client receives the echoed message, prints it, and frees the received buffer. So far that's the strategy I've seen in all the examples.

In both cases, that leaves the send buffers to be freed. I realized I couldn't free them immediately after calling uv_udp_send, since that just points to the buffer and doesn't send it right away. So I went to free it in the send callback, but unlike the receive and read cbs, you can't just free(buf->base). I tried free(req->bufs[0].base) similar to the write examples, but it appears to have already been freed and set to null in uv__udp_run_completed, before the callback is called. So that segfaults, but free(req->bufsml[0].base) works and gives no valgrind errors or leaks. I tried iterating over all non-null array entries in bufsml [ int i=0; while (req->bufsml[i].base) free(req->bufsml[i].base); ++i;  ] but that gives me one valgrind error per packet ("Conditional jump or move depends on uninitialised value(s)").

Am I doing this right?

I haven't been able to find any samples or examples of udp echo servers using libuv that even free memory in the send_cb other than free(req), but as far as I can tell, they all have memory leaks. The tcp server example in libuv-dox uses free(wr->buf.base) in its write_cb, but valgrind reports it has a huge memory leak (maybe it didn't 8 or 9 months ago with the version it was made for, but it does now). I don't know what the difference between bufs and bufsml is, and don't know if I should be concerned that I can't seem to check all members of the bufsml array to see if I should free them, only blindly free the first. It seems like this is harder than it should be.

[Saúl Ibarra Corretgé]

Hi Kevin,

On 09/17/2014 11:27 AM, Kevin Hendricks wrote:
> Hello Everyone,
>
> I only have a few days experience with libuv, but done a lot of reading
> of uv.h, the libuv-dox, and other resources, and I've gotten both a udp
> client and udp echo server running with just libuv. I understand all the
> memory management is on the user, which is an area I'm pretty green in.
> I'm using the current version of the master branch.
>

FYI, we have better docs now! http://docs.libuv.org
Also, I suggest to switch to using the v1.x branch.

> Both programs are fairly simple: the client opens a socket for sending
> and recieving, and while loops calls to uv_udp_send hundreds of char *
> packets ("packet 1," "packet 2," etc.), individually malloc'd, to the
> server. The server opens a socket for receiving, has a receive callback
> that prints the message, copies that into a newly allocated buffer,
> sends it back to the same addr it got it from, and frees() the received
> buf->base. The client receives the echoed message, prints it, and frees
> the received buffer. So far that's the strategy I've seen in all the
> examples.
>

That sounds ok.

> In both cases, that leaves the send buffers to be freed. I realized I
> couldn't free them immediately after calling uv_udp_send, since that
> just points to the buffer and doesn't send it right away. So I went to
> free it in the send callback, but unlike the receive and read cbs, you
> can't just free(buf->base). I tried free(req->bufs[0].base) similar to
> the write examples, but it appears to have already been freed and set to
> null in uv__udp_run_completed, before the callback is called. So that
> segfaults, but free(req->bufsml[0].base) works and gives no valgrind
> errors or leaks. I tried iterating over all non-null array entries in
> bufsml [ int i=0; while (req->bufsml[i].base) free(req->bufsml[i].base);
> ++i; ] but that gives me one valgrind error per packet ("Conditional
> jump or move depends on uninitialised value(s)").
>
> Am I doing this right?
>

Nope. libuv will copy the buffer structures you pass to uv_udp_send, but
not the content. That is left up to you to free.

> I haven't been able to find any samples or examples of udp echo servers
> using libuv that even free memory in the send_cb other than free(req),
> but as far as I can tell, they all have memory leaks. The tcp server
> example in libuv-dox uses free(wr->buf.base) in its write_cb, but
> valgrind reports it has a huge memory leak (maybe it didn't 8 or 9
> months ago with the version it was made for, but it does now). I don't
> know what the difference between bufs and bufsml is, and don't know if I
> should be concerned that I can't seem to check all members of the bufsml
> array to see if I should free them, only blindly free the first. It
> seems like this is harder than it should be.
>

The way to solve this elegantly is to embed the uv_udp_send_t structure
into another one:

typedef struct {
uv_udp_send_t req;
char *data;
} udp_send_ctx_t;

Then malloc one of these and pass &ctx->req to uv_udp_send. In the
callback, you can do the following to get a pointer to your context:

udp_send_ctx_t* ctx = container_of(req, udp_send_ctx_t, req);

And then you can just free ctx->data.


Cheers,

--
Saúl Ibarra Corretgé
bettercallsaghul.com

[Iñaki Baz Castillo]

2014-09-17 11:37 GMT+02:00 Saúl Ibarra Corretgé <sag...@gmail.com>:
> Then malloc one of these and pass &ctx->req to uv_udp_send. In the
> callback, you can do the following to get a pointer to your context:
>
> udp_send_ctx_t* ctx = container_of(req, udp_send_ctx_t, req);

Don't do that please. The "container_of" is just defined in Linux.
IMHO it is much more elegant to use the void* data field of the
uv_udp_send_t.

What I do in my C++ code (and in fact Saghul suggested it) is:


-------------------------------------
typedef struct UvSendData {
UDPSocket* socket; // My C++ UDPSocket class
uv_udp_send_t req;
uint8_t store[1];
} UvSendData;


void UDPSocket::Send(unint8_t* data, size_t len, const sockaddr* addr)
{
UvSendData* send_data = (UvSendData*)std::malloc(sizeof(UvSendData) + len);

send_data->socket = this;
memcpy(send_data->store, data, len);

// Store the pointer to our UvSendData in the data field
// of the uv_udp_send_t request:
send_data->req.data = (void*)send_data;

uv_buf_t buffer = uv_buf_init((char*)send_data->store, len);

int err = uv_udp_send(&send_data->req, this->uvUdpHandle, &buffer,
1, addr, (uv_udp_send_cb)on_send);

etc etc
}
-------------------------------------------


A single malloc is done. Then in the on_send callback:

------------------------------------------
static inline
void on_send(uv_udp_send_t* req, int status)
{
UDPSocket::UvSendData* send_data =
static_cast<UDPSocket::UvSendData*>(req->data);
UDPSocket* socket = send_data->socket;

// Delete the UvSendData struct (which includes the uv_req_t and the
store char[]).
free(send_data);

// Just notify the UDPSocket when error.
if (status)
socket->onUvSendError(status);
}
---------------------------------------------


Hope it helps.

--
Iñaki Baz Castillo
<i...@aliax.net>

[Saúl Ibarra Corretgé]

On 09/17/2014 11:55 AM, Iñaki Baz Castillo wrote:
> 2014-09-17 11:37 GMT+02:00 Saúl Ibarra Corretgé <sag...@gmail.com>:
>> Then malloc one of these and pass &ctx->req to uv_udp_send. In the
>> callback, you can do the following to get a pointer to your context:
>>
>> udp_send_ctx_t* ctx = container_of(req, udp_send_ctx_t, req);
>
>
> Don't do that please. The "container_of" is just defined in Linux.
> IMHO it is much more elegant to use the void* data field of the
> uv_udp_send_t.
>

Please elaborate, you didn't offer any proper reasoning as of why it's
not a good idea. "I don't like/understand it" is not a valid reason.

container_of is just a silly define that can be copied around if need
be. We use it all across libuv:
https://github.com/joyent/libuv/blob/master/src/uv-common.h#L45

> What I do in my C++ code (and in fact Saghul suggested it) is:
>

I don't remember when that was, but I've been using container_of for
quite a while now, and one you get it there is no looking back. The code
looks a lot more elegant and you avoid using that data pointer altogether.

[Iñaki Baz Castillo]

2014-09-17 12:00 GMT+02:00 Saúl Ibarra Corretgé <sag...@gmail.com>:
>> Don't do that please. The "container_of" is just defined in Linux.
>> IMHO it is much more elegant to use the void* data field of the
>> uv_udp_send_t.
>>
>
> Please elaborate, you didn't offer any proper reasoning as of why it's
> not a good idea. "I don't like/understand it" is not a valid reason.

You are basically doing (in uv_udp_send_cb):

udp_send_ctx_t* ctx = container_of(req, udp_send_ctx_t, req);

which becomes:

udp_send_ctx_t* ctx = ((udp_send_ctx*) ((char*) (req) -
offsetof(udp_send_ctx, req)));


I do:

- when sending:
&ctx->req.data = (void*)ctx;

- in uv_udp_send_cb:
udp_send_ctx_t* ctx = (udp_send_ctx_t*)req->data;


You require a ugly macro which relies on the offsetof() macro defined
for ANSI C, which may result in big issues when using with C++
objects:

-----------------------
-Wno-invalid-offsetof (C++ and Objective-C++ only)

Suppress warnings from applying the ‘offsetof’ macro to a non-POD type.

According to the 1998 ISO C++ standard, applying ‘offsetof’ to a
non-POD type is undefined. In existing C++ implementations, however,
‘offsetof’ typically gives meaningful results even when applied to
certain kinds of non-POD types. (Such as a simple ‘struct’ that fails
to be a POD type only by virtue of having a constructor.) This flag is
for users who are aware that they are writing nonportable code and who
have deliberately chosen to ignore the warning about it.

The restrictions on ‘offsetof’ may be relaxed in a future version of
the C++ standard.
-----------------------



Anyhow, may I know why your code is better than mine?

- Your code saves a void* assignement (wow!!!!).
- Mine does not use macros and uses UV public API.

[Iñaki Baz Castillo]

2014-09-17 12:18 GMT+02:00 Iñaki Baz Castillo <i...@aliax.net>:
> Anyhow, may I know why your code is better than mine?
>
> - Your code saves a void* assignement (wow!!!!).
> - Mine does not use macros and uses UV public API.

...and your code is dirty.

[Saúl Ibarra Corretgé]

On 09/17/2014 12:18 PM, Iñaki Baz Castillo wrote:
> 2014-09-17 12:00 GMT+02:00 Saúl Ibarra Corretgé <sag...@gmail.com>:
>>> Don't do that please. The "container_of" is just defined in Linux.
>>> IMHO it is much more elegant to use the void* data field of the
>>> uv_udp_send_t.
>>>
>>
>> Please elaborate, you didn't offer any proper reasoning as of why it's
>> not a good idea. "I don't like/understand it" is not a valid reason.
>
>
> You are basically doing (in uv_udp_send_cb):
>
> udp_send_ctx_t* ctx = container_of(req, udp_send_ctx_t, req);
>
> which becomes:
>
> udp_send_ctx_t* ctx = ((udp_send_ctx*) ((char*) (req) -
> offsetof(udp_send_ctx, req)));
>
>
> I do:
>
> - when sending:
> &ctx->req.data = (void*)ctx;
>
> - in uv_udp_send_cb:
> udp_send_ctx_t* ctx = (udp_send_ctx_t*)req->data;
>
>
> You require a ugly macro which relies on the offsetof() macro defined
> for ANSI C, which may result in big issues when using with C++
> objects:
>

There is an alternative ContainerOf thing in Node, for the curious.

libuv is a C library, so whatever I say works for C and a C compiler.
This might be the reason why I suggested you did it that way in the past.

Anyway, it may come down to a matter of taste, but dismissing my reply
because "it's ugly" and "only works on Linux" is deceiving, and incorrect.

[Iñaki Baz Castillo]

2014-09-17 12:26 GMT+02:00 Saúl Ibarra Corretgé <sag...@gmail.com>:
> Anyway, it may come down to a matter of taste, but dismissing my reply
> because "it's ugly" and "only works on Linux" is deceiving, and incorrect.

Sorry for that, my apologies. I just meant that it is "dirty" XD

[Kevin Hendricks]

Thank you both; this is exactly the kind of response I was hoping for! I will consider both options, and in either case, learning about the new API documentation not mentioned in the master readme should prove very useful. I'll try switching to the v1.x branch as well.