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.88k stars 213 forks source link

No reliable way for reclaiming memory with custom alloc callback #315

Open ruurdadema opened 2 months ago

ruurdadema commented 2 months ago

Thank you for this amazing library.

I'm gauging if uvw is suitable for low-latency & low-jitter media streaming over udp (using rtp). So far it looks quite promising.

One of the things I ran into is that there doesn't seem to be a reliable way of reclaiming memory which was previously allocated inside a custom allocator callback (link).

Inside the uvw::udp_handle::recv_callback, ownership of the allocated memory is taken by wrapping it with a std::unique_ptr (link). Although this is good practice, the udp_data_event is not triggered in all cases after that. When there is no more data to read or when there is a transmission error the udp_data_event is never called which makes it impossible to move the memory out of the std::unique_ptr. In practice the nread == 0 && addr == nullptr case happens a lot.

Unless I'm missing something (please let me know if this is the case), it would be really appreciated if you could add functionality that helps reclaiming memory in a consistent way. I'm thinking maybe adding an event type makes sense.

I'd also be willing to spend time on this and come up with a PR. Let me know if you'd prefer this.

skypjack commented 2 months ago

Is publishing an udp_data_event with a fake or default socket_address enough for the no more data case? I feel like it would work but you know it better than me probably since you've an use case already. 🙂 As for the transmission error 🤔 any suggestions? Not sure how we could solve it actually.

ruurdadema commented 2 months ago

Is publishing an udp_data_event with a fake or default socket_address enough for the no more data case?

Yes, that would do the job for the no more data case.

As for the transmission error 🤔 any suggestions? Not sure how we could solve it actually.

Normally I would suggest something like an deallocation callback, a chance for the user of the library to attach a callback in which previously allocated memory can be freed. This callback would be made at the end of recv_callback. However, since the data is moved into udp_data_event for the other cases it would make it all a bit inconsistent and maybe confusing.

Maybe it makes sense to fire a udp_data_event just before firing the error_event? (or the other way around).

Maybe consider adding a new type of event dealloc_event which gets fired at the end of recv_callback if the std::unique_ptr still holds data. If it doesn't then the data was already moved into the udp_data_event and there wouldn't be a reason to fire the dealloc_event. This would also avoid the need to fire a dummy udp_data_event for the no more data case.

The use case I have in mind is to have a pool of buffers and recycle those using the custom allocation callback (and the counterpart as discussed above). This pool could be global, thread local but I can also see reasons why this pool would need to be tied to the udp_handle itself. Unfortunately the udp_data_event doesn't give access to the handle, which could hold state using the user data facilities provided by the resource class, which is a superclass of udp_handle.

Another case to consider is where static buffers might be used for receiving data, similar to what libuv does in some of their tests and benchmarks link. When a custom callback assigns a pointer to a static buffer, the std::unique_ptr approach inside recv_callback is not a great fit. It can be worked around by using the .release() method on the std::unique_ptr but one has to be absolutely certain that the std::unique_ptr is neutralized, otherwise there will be a deallocation attempt on static memory (which will probably lead to a crash).

ruurdadema commented 4 weeks ago

I have come to the conclusion that for my project ASIO is a better fit, mainly because of it's low level capabilities. Unless this discussion is relevant or valuable to others, I propose closing this issue.