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 211 forks source link

uv_type: make `~uv_type()` destructor protected #303

Closed aloisklink closed 1 year ago

aloisklink commented 1 year ago

Make the destructor for uv_type protected, in order to the fix the -Wnon-virtual-dtor warnings:

‘struct uvw::uv_type<uv_async_s>’ has virtual functions and accessible non-virtual destructor [-Werror=non-virtual-dtor]

This prevents a potential memory leak when calling the ~uv_type() destructor (instead of the ~resource() destructor) on a uvw::resource.

Example of memory leak

As an example, the code below currently works (casting a pointer to a subclass of uvw::uv_type to uvw::uv_type, but it causes a memory leak, since only the uvw::uv_type destructor is called. Doing this is now forbidden.

uvw::resource<...>* will_leak;
auto abc = std::unique_ptr<uvw::uv_type>(will_leak);

BREAKING CHANGE: This change may break the uvw ABI, since we're changing the destructor from public to protected, which is fine on some platforms, but it's not guaranteed to be binary compatible. It also breaks the API, as some code that compiled previously (but with memory leaks), will now no longer compile.

I'd recommend waiting to merge this until the next MAJOR of uvw version (e.g. uvw v4).

skypjack commented 1 year ago

What about making it virtual? Why did you make it protected instead? I don't see any advantages and I prefer non-breaking changes (put aside the ABI ofc).

aloisklink commented 1 year ago

What about making it virtual? Why did you make it protected instead? I don't see any advantages and I prefer non-breaking changes (put aside the ABI ofc).

Making a function virtual has a performance hit (although a very small performance hit!), and although it won't break any APIs, it would break the ABI anyway, so it will still require a MAJOR uvw version bump since uvw has shared libraries.

I also think that the chance of using uvw::uv_type directly is pretty rare, so even if it is an API breaking change, I think there is a pretty low chance it would affect anybody's code:

// does anybody actually use `uvw::uv_type` pointers??
uvw::uv_type<x>* base_ptr = new uvw::resource<...>();
del base_ptr; // this is currently undefined behavior, and will probably memory leak

I did a search on GitHub for people using "uvw::uv_type" and I couldn't find anything, other than this PR, see https://github.com/search?type=code&q=%22uvw%3A%3Auv_type%22.

So to summarize, I think pretty much nobody is using uvw::uv_type directly (and if they, it's causing memory leaks). But if somebody is using it, making the function protected will stop their code from compiling. But we can make the function virtual (with a small performance hit), if you want. It's up to you :)