[bisq-network/bisq] Fix memory leak caused by not cleaning up unused threads (#3734)

[Florian Reimair]

> What was the memory footprint change after running with your patch?

Just as I attempted to get numbers, jProfiler crashed after a 48h test run and with it my bisq-under-test and the whole gnome shell. I did, however, see the fix doing its job. All in all, I decided to not waste another couple of days to rerun anything but put the PR up for merging as this is a real bug that needs fixing.

> One leaked executor per connection does seem bad, but it would be good to understand the full impact.

due to the executors not being shut down, these threads stayed alive and with them, the parent threads per connection as well. Over a couple of days this behavior lead to hundreds of (unused) threads. But only if there is high network load and the `Bundle_of_envelopes` feature triggered.
Overall, the memory-leak part hasn't been that bad as it seems (there isn't that much change in a clients connection pool) but I have seen increased CPU usage as well - although I have no hard evidence that this is related.

A JVM profiler showed me that there has been a huge number of threads all in waiting-state. So I tracked down their source, found that there is indeed a bug causing a "thread-leak" and fixed it. My guess is that with the recently forced update, the bundle-of-envelopes stuff finally went really active as all clients made use of this.

I honestly do not know if that bug is responsible for some resource-related issues reported recently. There has been incorrect behavior and it is fixed now. I would leave it for time to tell if it has been that great of an impact in the first place.

> Any additional information on which test cases may need to be re-run to verify everything works as expected if this goes into v1.2.4?

As always with our P2P part, there is no exhaustive testing possible right now. Since the change only cleanes up stuff, there should be no need for any testplan testcases to be redone. However, I suggest to maybe redo the trading tests just to be sure.

> Any screenshots or tools we can use to help verify the new behavior?

Well take a JVM profiler, start up 1.2.3 and this fix and compare the number of threads after 2 days of runtime. With the fix, there should be substantially less threads. Please note that I am not entirely confident that there isn't another "thread-leak" somewhere, still. But that is for another day.

> One leaked executor per connection does seem bad, but it would be good to understand the full impact.

Agreed. Generally, thread handling and synchronization is a mess right now. Needs fixing sometime.



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3734#issuecomment-561410909
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191203/0ebf0d6e/attachment.html>