Avoid memory leaks in RMQConnection dependency graph (take 4) - Githubissues

rabbitmq / rabbitmq-objc-client

RabbitMQ client for Objective-C and Swift

https://rabbitmq.com

Other

242 stars 84 forks source link

Avoid memory leaks in RMQConnection dependency graph (take 4) #198

Closed BarryDuggan closed 2 years ago

BarryDuggan commented 2 years ago

Proposed Changes

This PR is a work in progress. It is an attempt to solve memory leaks in this client. These memory leaks can occur when we close a connection, make it nil, and then attempt to create a new connection. In this case we would expect to have 1 connection, but in fact we have 2. See this discussion for more details.

Change Made

Added a new Memory Test Target to make it easier the run memory graph against changes made.
Broke several retain cycles by using weak references.
Close channels and nil references when closing connections

This PR is verified by all unit tests passing.

That said it will need as many eyes on it as possible because so many areas of the code base have been touched.

pivotal-cla commented 2 years ago

@BarryDuggan Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-cla commented 2 years ago

@BarryDuggan Thank you for signing the Contributor License Agreement!

BarryDuggan commented 2 years ago

@michaelklishin I've updated the branch to close channels when closing a connection. There's now no memory leaks and the tests are passing.

I'm cautiously optimistic that we're headed in the right direction.....

michaelklishin commented 2 years ago

OK. Closing all channels is not necessary when a connection is closed, as they won't be useable any more. We can "close" (release) them without going through the channel.close — channel.close-ok network roundtrip but some clients do that and it's OK since most connections don't have a lot of channels open on them.

michaelklishin commented 2 years ago

@BarryDuggan all tests pass 10 runs in a row 👍 We need to add the new memory leak tests to the list of targets executed with gmake and gmake tests_{ios,osx}, otherwise I have no objections to merging this.

michaelklishin commented 2 years ago

Looks like MemoryTestTests and friends are just stubs produced by XCode? Do we care to implement them (this kind of tests can be tricky to get right) or should they be removed?

BarryDuggan commented 2 years ago

Looks like MemoryTestTests and friends are just stubs produced by XCode? Do we care to implement them (this kind of tests can be tricky to get right) or should they be removed?

We can delete them. I'll just add a simple check for nil test in the existing test suite instead.

michaelklishin commented 2 years ago

Perfect. Let's do that and merge then :) Thank you!

BarryDuggan commented 2 years ago

@michaelklishin will we be bumping the cocoapods versions / making a release for these changes?

michaelklishin commented 2 years ago

Yes, some time later this week. I haven't done a release in a good year so need to re-learn how that's done.

michaelklishin commented 2 years ago

It took me a while but 0.12.0 has been published to CocoaPods Trunk.

@BarryDuggan let me know if I've missed anything.

axel012 commented 11 months ago

It took me a while but 0.12.0 has been published to CocoaPods Trunk.

@BarryDuggan let me know if I've missed anything.

Found bug with last change, using weak for heartBeatSender is causing to be dealocated before calling start causing disconnects issues since the heartbeat is nil.

Steps to reproduce:

Start a new connection calling any init function

In the image below we watch _heartBeatSender variable for changes between the init flow, the value is set but then its nil again when the instance is created.

michaelklishin commented 11 months ago

@axel012 please submit a PR. Thank you.