Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ruurdadema opened this issue Sep 3, 2024 · 2 comments
Open
Assignees
Labels

Comments

@ruurdadema
Copy link
Contributor

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 skypjack self-assigned this Sep 3, 2024
@skypjack skypjack added the triage label Sep 3, 2024
@skypjack
Copy link
Owner

skypjack commented Sep 3, 2024

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
Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants