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

Document suggested cleanup approach #307

Closed rconde01 closed 1 year ago

rconde01 commented 1 year ago

On Windows I was seeing memory leaks for a simple case. Turned out that you need to run the loop to actually clean stuff up. I ended up with this approach:

      auto loop = uvw::loop::get_default();

      // ...

      loop->run(); // stops at some point

      while (loop->close() == UV_EBUSY) {
         loop->walk([](auto&& h) {
            if (!h.closing())
               h.close();
         });

         loop->run(uvw::loop::run_mode::ONCE);
      }

It would be good to know if this approach is reasonable, and if so perhaps add it to the docs (potentially with some max wait time).

rconde01 commented 1 year ago

This is definitely not quite right - getting access violations sometimes

skypjack commented 1 year ago

It's impossible to tell what you're doing wrong without a repro. As far as I can see, you can have tons of UBs everywhere. 🤷‍♂️ That being said, uvw stays true to the requirements of libuv and the latter requires users to close handles before leaving.

rconde01 commented 1 year ago

I apologize...here's a complete example:

#include <uvw.hpp>
#include <crtdbg.h>
#include <iostream>

auto setup_leak_detection() -> void;

auto main() -> int {
    setup_leak_detection();

    // Simple loop
    auto loop = uvw::loop::get_default();

    auto socket = loop->resource<uvw::pipe_handle>();

    loop->walk([](auto&& h) {
        h.close();
    });
}

auto alloc_hook(
   int /*allocType*/,
   void* /*userData*/,
   size_t size,
   int /*blockType*/,
   long request_number,
   unsigned char const* /*filename*/,
   int /*lineNumber*/) -> int {

   // a memory leak can jump around a bit run to run
   // so we define a window around the reported allocation
   // number

   int       nominal_request = 163;
   int       leak_size       = 200;
   int const window_size     = 20;

   if (
      request_number >= nominal_request - window_size &&
      request_number <= nominal_request + window_size && size == leak_size)
   {
      // set a breakpoint here to find the allocation
      int five = 5;
   }

   return true;
}

auto setup_leak_detection() -> void {
   auto initial_flag = _CrtSetDbgFlag(_CRTDBG_REPORT_FLAG);
   _CrtSetDbgFlag(initial_flag | _CRTDBG_LEAK_CHECK_DF);
   _CrtSetAllocHook(alloc_hook);
   _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
   _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);
   _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
   _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDOUT);
   _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
   _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDOUT);
}

and here's the output:

Detected memory leaks!
Dumping objects ->
{168} normal block at 0x0000019C8710C6C0, 1192 bytes long.
 Data: <h               > 68 DD 97 C5 F7 7F 00 00 01 00 00 00 02 00 00 00
{167} normal block at 0x0000019C870F91B0, 24 bytes long.
 Data: <8               > 38 DD 97 C5 F7 7F 00 00 01 00 00 00 02 00 00 00
{166} normal block at 0x0000019C870E6560, 120 bytes long.
 Data: <0               > 30 DC 97 C5 F7 7F 00 00 CD CD CD CD CD CD CD CD
{165} normal block at 0x0000019C870E3740, 64 bytes long.
 Data: <                > 80 17 9C C5 F7 7F 00 00 00 00 00 00 00 00 00 00
{164} normal block at 0x0000019C870FE130, 16 bytes long.
 Data: <                > 00 00 00 00 00 00 00 00 00 00 00 00 CD CD CD CD
{163} normal block at 0x0000019C87104390, 200 bytes long.
 Data: <                > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Object dump complete.
rconde01 commented 1 year ago

ok...this seems to work:

auto main() -> int {
    setup_leak_detection();

    // Simple loop
    auto loop = uvw::loop::get_default();

    auto socket = loop->resource<uvw::pipe_handle>();

    loop->walk([](auto&& h) {
        h.close();
    });

    loop->run();
}
skypjack commented 1 year ago

That's because you run the loop for another tick after closing your handles. If you don't do that, handles aren't cleaned up properly and thus the leak. It boils down to libuv and how it works though. uvw stays true to this requirement as mentioned. 👍

rconde01 commented 1 year ago

Sure - I get you probably don't want to reproduce the libuv docs, but I would suggest you update your "Code Example" to:

int main() {
    auto loop = uvw::loop::get_default();
    listen(*loop);
    conn(*loop);
    loop->run();

    loop->walk([](auto&& h) {
        h.close();
    });

    loop->run();   
}

Since it seems that's the canonical approach people should use...but it's your library :)

skypjack commented 1 year ago

Which code example? Do you mean this one?

rconde01 commented 1 year ago

yes - although optionally to your tests as well (libuv tests do something similar).

skypjack commented 1 year ago

The test in the README file already closes handles as requested. I don't think there is nothing to fix. Thanks for the heads-up though. I'm glad you found (and fixed) the issue. 👍