Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Apr 4, 2022

Conversation

BarryDuggan
Copy link
Contributor

@BarryDuggan BarryDuggan commented Apr 1, 2022

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
Copy link

@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
Copy link

@BarryDuggan Thank you for signing the Contributor License Agreement!

Calling close on channels when closing connection
@BarryDuggan
Copy link
Contributor Author

BarryDuggan commented Apr 1, 2022

@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.....

@BarryDuggan BarryDuggan requested a review from camelpunch April 1, 2022 19:31
@michaelklishin
Copy link
Contributor

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.closechannel.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 michaelklishin removed the request for review from camelpunch April 1, 2022 20:34
@michaelklishin michaelklishin changed the title Rabbit obj client 194 take 4 Avoid memory leaks in RMQConnection dependency graph (take 4) Apr 1, 2022
@michaelklishin
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@BarryDuggan BarryDuggan marked this pull request as ready for review April 4, 2022 10:32
@michaelklishin michaelklishin merged commit 91787b9 into master Apr 4, 2022
@michaelklishin michaelklishin deleted the rabbit-obj-client-194-take-4 branch April 4, 2022 11:25
@BarryDuggan
Copy link
Contributor Author

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

@michaelklishin
Copy link
Contributor

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
Copy link
Contributor

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

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

@michaelklishin michaelklishin added this to the v0.12.0 milestone Apr 24, 2022
@axel012
Copy link

axel012 commented Jul 31, 2023

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.
image

@michaelklishin
Copy link
Contributor

@axel012 please submit a PR. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants