CopernicaMarketingSoftware / AMQP-CPP

C++ library for asynchronous non-blocking communication with RabbitMQ
Apache License 2.0
884 stars 340 forks source link

AMQP::LibBoostAsioHandler::Watcher "memory leak" #238

Open msukalski opened 6 years ago

msukalski commented 6 years ago

The Watcher code uses 2 boolean flags, _read_pending and _write_pending, to attempt to prevent more than one read and write ASIO operation from being posted at a time. Unfortunately, the existing implementation is not thread-safe, since changes in the boolean values are not necessarily visible to all threads at the same time. The net result is that the ASIO queues end up with requests that are never processed, and it appears like a slow but always growing memory leak that will eventually consume all available memory.

I've attached my changes to these flags (std::atomic ...) that require C++11 support. compare_exchange_strong() is used to prevent more than one thread from posting a read or write operation at a time. This version of libboostasio.h has also been modified for using asio in standalone mode (and all other boost support has been removed through the use of lambdas everywhere). This code has been tested and verified. There are other ways of addressing this issue, of course, so your mileage may vary...

libboostasio.h.txt

EHadoux commented 6 years ago

Is it related to #237? Because after closing the connection watchers are all that is left.

msukalski commented 6 years ago

No, it doesn't appear to be. You can watch your memory usage grow as "extra" asio read and write operations are posted with a stable connection with a single Watcher. The issue appears because multiple independent threads end up posting asio stream async_read_some() and async_write_calls() -- while only a single operation on an asio strand is dispatched at a time.

zerodefect commented 6 years ago

Can I confirm that once you introduced atomic read/writes on the pending variables that the memory leak(s) (you are reporting) ceased?

mihacooper commented 5 years ago

I have found the similar problem with AMQP::LibBoostAsioHandler. This is my valgrind's massif output asio_memory_leak

Patch proposed by @msukalski looks like fix the problem. asio_memory_fix

I have made a real patch from the posted code: libboostasio.h.patch.txt

But I have nothing to say about its correctness and workability.

zerodefect commented 5 years ago

Ok. Good to know. I'll look at this evening. Thanks for your investigative work.

mvdwerve commented 5 years ago

Let me interject for a second. Nothing in AMQP-CPP is meant to be thread-safe. Generally, you should think in terms of connections, and if you do happen to want to share a connection over threads, or have a general thread-safe handler, you should probably write it yourself.

Mostly, the problem is not processing power but rather waiting for IO, which means that a handler running on a single thread can easily manage multiple connections. That some objects just happen to work well across threads or are thread-safe (like the boost asio service) cannot be assumed about other things in the library. Most likely, if you are using it this way, you'll have more issues than just the libboost handler.

I'll add a general remark towards this end in the readme, about threaded usage overall. Also, the libboost handler is not supported by us, it even says so in the file. Just saying.