skypjack / uvw

Header-only, event based, tiny and easy to use libuv wrapper in modern C++ - now available as also shared/static library!
MIT License
1.84k stars 209 forks source link

Support for recvmmsg seems to be broken #235

Closed sourexx closed 3 years ago

sourexx commented 3 years ago

Hello,

I've been trying to get recvmmsg working, and encountered several issues:

  1. To enable recvmmsg mode in libuv, UV_UDP_RECVMMSG flag should be passed to uv_udp_init_ex().

    UVW does have flag uvw::UDPHandle::Bind::UDP_RECVMMSG, but calling bind method on UDPHandle with the flag:

    auto udp = m_loop.resource<uvw::UDPHandle>();
    udp->bind(m_bind_endpoint, uvw::UDPHandle::Bind::UDP_RECVMMSG);

    apparently leads to a bind error in libuv.

    While passing libuv's flag directly at UDPHandle construction works fine:

    auto udp = m_loop.resource<uvw::UDPHandle>(UV_UDP_RECVMMSG);

    That's a bit of confusion I've encountered foremost.

  2. More important issue involves memory buffers management in UVW.

    In recvmmsg mode libuv expects to get from alloc callback the buffer large enough to receive multiple UDP datagrams with a single recvmmsg syscall.

    Then libuv splits the buffer in chunks, calls recvmmsg() and returns chunks to recv callback one by one.

    UVW allocates memory buffer in Handle::allocCallback with new char[] and passes array pointer to libuv, then in receive callback UDPHandle::recvCallback() wraps libuv-supplied pointer with unique_ptr. As the pointer remains the same, this works perfectly fine (i.e. recvmsg). With larger buffer and recvmmsg this leads to delete[] on every returned chunk. This obviously leads to crashes.

As far as I can see at the moment, there is no way to avoid copying in UDPHandle::recvCallback() for chunk calls (i.e with UV_UDP_MMSG_CHUNK flag set). Which will sacrifice receive efficiency, the whole point of recvmmsg is for.

The other possible approach may be to preallocate receive buffer once and do not wrap returned pointers with unique_ptr in UDPHandle::recvCallback(). That generally may be implemented with some memory management policy on top of uvw::Handle, in order not to brake backward compatibility.

Are there any considerations on the issue?

And sorry for getting a bit wordy.

Best regards, Sergio

skypjack commented 3 years ago

As for 1:

That's a bit of confusion I've encountered foremost.

I see what you mean. On the other side, uvw stays true to the API of libuv and from what I can see this confusion is inherited from the underlying library. Am I wrong?

As for 2, I'm not sure I get how you would add a memory management policy on top of the handle. Could you elaborate a little further with some technical details to see if it fits as expected? It would help to follow your reasoning. Thanks.

And sorry for getting a bit wordy.

No worries. You're welcome. 🙂

skypjack commented 3 years ago

As a side note, is this also related to #223 ?

sourexx commented 3 years ago

Michele, thanks for your response!

Yep, seems to be related to #223.

  1. IMO, it feels reasonable, if uvw would expose to user API those flags that actualy modifies libuv's behavior (UV_UDP_IPV6ONLY, UV_UDP_REUSEADDR, UV_UDP_RECVMMSG).

    And would not expose flags that supposed to be handled in uvw's internal methods (e.g. UDPHandle::recvCallback()), as uvw's user would not have a chance to interact with those flags (UV_UDP_PARTIAL, UV_UDP_MMSG_CHUNK, UV_UDP_MMSG_FREE).

    As for UV_UDP_RECVMMSG, IMHO, it should not belong to uvw::UDPHandle::Bind namespace to avoid confusion. Or better should not be exposed at all and set automatically with uvw::UDPHandle's receive policy.

    // Keep using recvmsg as default
    auto udp = loop.resource<uvw::UDPHandle>();
    
    // Switch to recvmmsg and set UV_UDP_RECVMMSG flag if stated explicitly
    auto udp_mmsg = loop.resource<uvw::UDPHandle<uvw::UDPMultiple>>();

    Receive policy could also affect default memory management policy. What do you think on that?

  2. On memory management policy, I'd have to investigate a little bit deeper to introduce an MVP for that.

    One thing I don't quite understand, that if buffer ownership semantics in UDPDataEvent doesn't works well with recvmmsg (as libuv internally splits provided buffer in chunks), should UDPHandle's mmsg mode publish some other event type, that doesn't call delete[] on chunks? Would it be appropriate, if that type would depend on selected memory management policy?

    Or is there any better approach?

skypjack commented 3 years ago

1. What do you think on that?

Not sure I follow your reasoning, but isn't this problem also part of the libuv API? How did they solve it, if they solved it at all? As far as I remember, I mimic the API of the underlying library for all functions. So, either I missed something here or the problem is down the tube.

2.

The callback is the same in all cases, so my guess is that we cannot really differentiate it when it comes at sending the message. Though, I may be wrong, I don't remember the details of this part in libuv. Does it make sense to you?

skypjack commented 3 years ago

Ping. 🙂

sourexx commented 3 years ago

Sorry for the delay, I'm going to return to this issue on weekend.

skypjack commented 3 years ago

Stale issue. Closing this because of a lack of information. Feel free to reopen the issue if and when you find the time to dig a little deeper into the problem. Thanks.