RMQConnection leaking memory when recreated

[BarryDuggan]

RMQConnection leaking memory when recreated.

Steps to reproduce

In Xcode Set Live Allocation Logging

• Create a RMQConnection Storing a weak reference
• Close the RMQConnection
• Nil the RMQ Connection
• Create a new RMQConnection

Repeat steps 1 - 4 a few times
Select Memory Graph in Xcode to View Memory Warnings

Expected Results

1 instance of RMQConnection Will Exist

Actual Results

Multiple instances of RMQConnection Exist.
Many Memory Leaks reported in Xcode.

Sample Code

import UIKit
import RMQClientNC

class ViewController: UIViewController {
    let amqp = "insert-rmq-url-here"
    fileprivate var connection: RMQConnection?
    fileprivate var channel: RMQChannel?
    fileprivate var receivingChannel: RMQChannel?
    fileprivate var consumer: RMQConsumer?
    fileprivate var queue: RMQQueue?
    fileprivate let delegateQueue = DispatchQueue(label: "rmq-test-queue")
    override func viewDidLoad() {
        super.viewDidLoad()
        startup()
    }

    @IBAction func restartConnection() {
        print("Restarting")
        self.connection?.close()
        self.connection = nil
        self.connection?.delegate = nil
        self.receivingChannel = nil
        self.channel = nil
        self.consumer = nil
        self.startup()
    }
    /// start the connection and subscribe to the exchange
    func startup() {
        weak var this = self
        if connection == nil {
            connection = RMQConnection(uri: amqp, delegate: this)
            connection?.start()
        }
    }

}

extension ViewController: RMQConnectionDelegate {
    /// @brief Called when a socket cannot be opened, or when AMQP handshaking times out for some reason.
    func connection(_ connection: RMQConnection!, failedToConnectWithError error: Error!) {
        print(self)
        print("RABBIT: failedToConnectWithError")
    }

    /// @brief Called when a connection disconnects for any reason
    func connection(_ connection: RMQConnection!, disconnectedWithError error: Error!) {
        print(self)
        print("RABBIT: disconnectedWithError")
    }

    /// @brief Called before the configured http://www.rabbitmq.com/api-guide.html#recovery automatic connection recovery</a> sleep.
    func willStartRecovery(with connection: RMQConnection!) {
        print(self)
        print("RABBIT: willStartRecovery")
    }

    /// @brief Called after the configured http://www.rabbitmq.com/api-guide.html#recovery automatic connection recovery</a> sleep.
    func startingRecovery(with connection: RMQConnection!) {
        print(self)
        print("RABBIT: startingRecovery")
    }

    /*!
     * @brief Called when http://www.rabbitmq.com/api-guide.html#recovery automatic connection recovery</a> has succeeded.
     * @param RMQConnection the connection instance that was recovered.
     */
    func recoveredConnection(_ connection: RMQConnection!) {
        print(self)
        print("RABBIT: recoveredConnection")

    }

    /// @brief Called with any channel-level AMQP exception.
    func channel(_ channel: RMQChannel!, error: Error!) {
        print(self)
        print("RABBIT: Channel Error")
    }
}

[michaelklishin]

@BarryDuggan I don't see any evidence of a problem in the client. It does not instantiate or keep references to RMQConnection, your own code does.

You are welcome to investigate this and provide evidence and recommendations (or simply submit a PR), this is open source software after all.

[BarryDuggan]

Hi and Thanks for the reply.
I disagree that there is no problem with the client.
The sample code I posted is the bare bones example given in the read me with the exception of recreating the connection.

Are there any special considerations that you can think of for cases where we want to close a connection and create a new one? (I could be just using the library wrong or asking it to do something it's not designed to do)

You can see from the attached memory graph that there are numerous strong references between the closed RMQConnection and other objects.

I'm far from an expert in this framework or RMQ in general.
That said I will spend some time investigating and attempt to provide more compelling evidence / propose a solution.

[michaelklishin]

Thanks. I only see a loop on this diagram if a RMQAllocatedChannel is involved but no (even distantly modern) clients should open channels by default.

[michaelklishin]

Ah, it must be the _channelZero then, which is not a "real channel" in many clients as it is used for certain "system" (low level) messages in the protocol. I wonder if we can make that reference weak and that would break the reference loop?

[BarryDuggan]

Here is another more contrived example to illustrate that there is nothing in the app holding a reference to the RMQConnection.
After running this code I would expect there to be 0 instances of RMQConnection.
After running this code there is 3 instances of RMQConnection.

func startup() {
        var  connection1:RMQConnection? = RMQConnection(uri: amqp, delegate: self)
        connection1?.start()
        connection1?.close()
        connection1 = nil
        var  connection2:RMQConnection? = RMQConnection(uri: amqp, delegate: self)
        connection2?.start()
        connection2?.close()
        connection2 = nil
        var  connection3:RMQConnection? = RMQConnection(uri: amqp, delegate: self)
        connection3?.start()
        connection3?.close()
        connection3 = nil
    }

[BarryDuggan]

@michaelklishin
I think I have solved the retain cycle.
The problem is I can't create a PR for review because I can't get the project to build.
I get followed the guide here but I get a ton of errors (please see screen shot).

Below are the changes I made locally.
It's mostly defining some of the properties as weak.
For example the dispatcher is passed to the RMQAllocatedChannel so RMQAllocatedChannel does not 'own' dispatcher so I made it weak.

RMQAllocatedChannel

@interface RMQAllocatedChannel ()
@property (nonatomic, copy, readwrite) NSNumber *channelNumber;
@property (nonatomic, readwrite) NSNumber *contentBodySize;
@property (nonatomic,weak, readwrite) id <RMQDispatcher> dispatcher;
@property (nonatomic, readwrite) NSMutableDictionary *consumers;
@property (nonatomic, readwrite) NSMutableDictionary *exchanges;
@property (nonatomic, readwrite) NSMutableDictionary *exchangeBindings;
@property (nonatomic, readwrite) NSMutableDictionary *queues;
@property (nonatomic, readwrite) NSMutableDictionary *queueBindings;
@property (nonatomic, readwrite) id<RMQConfirmations> confirmations;
@property (nonatomic, readwrite) NSNumber *prefetchCountPerConsumer;
@property (nonatomic, readwrite) NSNumber *prefetchCountPerChannel;
@property (nonatomic,weak, readwrite) id<RMQConnectionDelegate> delegate;
@property (nonatomic, readwrite) id<RMQNameGenerator> nameGenerator;
@property (nonatomic,weak, readwrite) id<RMQChannelAllocator> allocator;
@end

RMQConnection

@property (weak, nonatomic, readwrite) id <RMQTransport> transport;
@property (nonatomic,weak, readwrite) RMQReader *reader;
@property (nonatomic,weak, readwrite) id <RMQChannelAllocator> channelAllocator;
@property (nonatomic,weak, readwrite) id <RMQFrameHandler> frameHandler;
@property (nonatomic, readwrite) id<RMQLocalSerialQueue> commandQueue;
@property (nonatomic, readwrite) id<RMQWaiterFactory> waiterFactory;
@property (nonatomic,weak, readwrite) id<RMQHeartbeatSender> heartbeatSender;
@property (nonatomic, readwrite) id <RMQChannel> channelZero;
@property (nonatomic,weak, readwrite) RMQConnectionConfig *config;
@property (nonatomic, readwrite) NSMutableDictionary *userChannels;
@property (nonatomic, readwrite) NSNumber *frameMax;
@property (nonatomic, readwrite) BOOL handshakeComplete;
@property (nonatomic, readwrite) NSNumber *handshakeTimeout;

My Errors

[michaelklishin]

I am deep in an, ahem, rabbit hole with angled vs. quoted #imports with different targets telling me to use different things.

Running all tests involves setting up a RabbitMQ node with a specific set up plugins and using

• export SBIN="/path/to/rabbitmq/for_objc/sbin"; gmake before_build RABBITMQCTL="$SBIN/rabbitmqctl" RABBITMQ_PLUGINS="$SBIN/rabbitmq-plugins" to set things up on the node
• gmake to run the tests

but you don't have to worry about that.

[BarryDuggan]

Apologies about the surplus weak references.
Below is the bare minimum that I have used.

No compiler warnings.
No issue with establishing and maintaining connections.
No Retain Cycle.
No Dropped Delegates.

@interface RMQAllocatedChannel ()
@property (nonatomic, copy, readwrite) NSNumber *channelNumber;
@property (nonatomic, readwrite) NSNumber *contentBodySize;
@property (nonatomic, readwrite) NSMutableDictionary *consumers;
@property (nonatomic, readwrite) NSMutableDictionary *exchanges;
@property (nonatomic, readwrite) NSMutableDictionary *exchangeBindings;
@property (nonatomic, readwrite) NSMutableDictionary *queues;
@property (nonatomic, readwrite) NSMutableDictionary *queueBindings;
@property (nonatomic, readwrite) id<RMQConfirmations> confirmations;
@property (nonatomic, readwrite) NSNumber *prefetchCountPerConsumer;
@property (nonatomic, readwrite) NSNumber *prefetchCountPerChannel;
@property (nonatomic, readwrite) id<RMQConnectionDelegate> delegate;
@property (nonatomic, readwrite) id<RMQNameGenerator> nameGenerator;
@property (nonatomic,weak, readwrite) id<RMQChannelAllocator> allocator;
@property (nonatomic,weak, readwrite) id <RMQDispatcher> dispatcher;
@end

@interface RMQConnection ()
@property ( nonatomic, readwrite) id <RMQTransport> transport;
@property (nonatomic,weak, readwrite) RMQReader *reader;
@property (nonatomic,weak, readwrite) id <RMQChannelAllocator> channelAllocator;
@property (nonatomic,weak, readwrite) id <RMQFrameHandler> frameHandler;
@property (nonatomic, readwrite) id<RMQHeartbeatSender> heartbeatSender;
@property (nonatomic, readwrite) id <RMQChannel> channelZero;
@property (nonatomic, readwrite) RMQConnectionConfig *config;
@property (nonatomic, readwrite) id<RMQLocalSerialQueue> commandQueue;
@property (nonatomic, readwrite) id<RMQWaiterFactory> waiterFactory;
@property (nonatomic, readwrite) NSMutableDictionary *userChannels;
@property (nonatomic, readwrite) NSNumber *frameMax;
@property (nonatomic, readwrite) BOOL handshakeComplete;
@property (nonatomic, readwrite) NSNumber *handshakeTimeout;
@end

[michaelklishin]

This again seems to drop the delegate property from RMQConnection.

[michaelklishin]

It also introduces an ARC semantics issue:

RMQClient/RMQConnection.m:119:21: Assigning retained object to weak property; object will be released after assignment

Here is the line:

self.reader = [[RMQReader alloc] initWithTransport:self.transport frameHandler:self];

Does reader have to be weak since it is an essential property fully managed internally?

[BarryDuggan]

It's strange that I don't see that compiler warning.
(I wonder why that is?)
If I make Reader strong by itself the connection still leaks.

[michaelklishin]

@BarryDuggan does this diff look correct to you? I'd be happy to create a PR (with all credit given to you) and run test suites locally with it.

diff --git a/RMQClient/RMQAllocatedChannel.m b/RMQClient/RMQAllocatedChannel.m
index eaa4df7..45bdfca 100644
--- a/RMQClient/RMQAllocatedChannel.m
+++ b/RMQClient/RMQAllocatedChannel.m
@@ -49,7 +49,7 @@
 @interface RMQAllocatedChannel ()
 @property (nonatomic, copy, readwrite) NSNumber *channelNumber;
 @property (nonatomic, readwrite) NSNumber *contentBodySize;
-@property (nonatomic, readwrite) id <RMQDispatcher> dispatcher;
+@property (nonatomic, weak, readwrite) id <RMQDispatcher> dispatcher;
 @property (nonatomic, readwrite) NSMutableDictionary *consumers;
 @property (nonatomic, readwrite) NSMutableDictionary *exchanges;
 @property (nonatomic, readwrite) NSMutableDictionary *exchangeBindings;
@@ -58,9 +58,9 @@
 @property (nonatomic, readwrite) id<RMQConfirmations> confirmations;
 @property (nonatomic, readwrite) NSNumber *prefetchCountPerConsumer;
 @property (nonatomic, readwrite) NSNumber *prefetchCountPerChannel;
-@property (nonatomic, readwrite) id<RMQConnectionDelegate> delegate;
+@property (nonatomic, weak, readwrite) id<RMQConnectionDelegate> delegate;
 @property (nonatomic, readwrite) id<RMQNameGenerator> nameGenerator;
-@property (nonatomic, readwrite) id<RMQChannelAllocator> allocator;
+@property (nonatomic, weak, readwrite) id<RMQChannelAllocator> allocator;
 @end
 
 @implementation RMQAllocatedChannel
diff --git a/RMQClient/RMQConnection.m b/RMQClient/RMQConnection.m
index 2a0aafc..1930c9e 100644
--- a/RMQClient/RMQConnection.m
+++ b/RMQClient/RMQConnection.m
@@ -60,22 +60,19 @@
 #import <RMQFrame.h>
 #import <RMQProcessInfoNameGenerator.h>
 
-@interface RMQConnection ()
-@property (strong, nonatomic, readwrite) id <RMQTransport> transport;
-@property (nonatomic, readwrite) RMQReader *reader;
-@property (nonatomic, readwrite) id <RMQChannelAllocator> channelAllocator;
-@property (nonatomic, readwrite) id <RMQFrameHandler> frameHandler;
+@property (weak, nonatomic, readwrite) id <RMQTransport> transport;
+@property (nonatomic, weak, readwrite) RMQReader *reader;
+@property (nonatomic, weak, readwrite) id <RMQChannelAllocator> channelAllocator;
+@property (nonatomic, weak, readwrite) id <RMQFrameHandler> frameHandler;
 @property (nonatomic, readwrite) id<RMQLocalSerialQueue> commandQueue;
 @property (nonatomic, readwrite) id<RMQWaiterFactory> waiterFactory;
-@property (nonatomic, readwrite) id<RMQHeartbeatSender> heartbeatSender;
-@property (nonatomic, weak, readwrite) id<RMQConnectionDelegate> delegate;
+@property (nonatomic, weak, readwrite) id<RMQHeartbeatSender> heartbeatSender;
 @property (nonatomic, readwrite) id <RMQChannel> channelZero;
-@property (nonatomic, readwrite) RMQConnectionConfig *config;
+@property (nonatomic, weak, readwrite) RMQConnectionConfig *config;
 @property (nonatomic, readwrite) NSMutableDictionary *userChannels;
 @property (nonatomic, readwrite) NSNumber *frameMax;
 @property (nonatomic, readwrite) BOOL handshakeComplete;
 @property (nonatomic, readwrite) NSNumber *handshakeTimeout;
-@end
 
 __attribute__((constructor))
 static void RMQInitConnectionConfigDefaults() {

[BarryDuggan]

Yeah that diff is 100% correct.
Sorry I couldn't make the PR myself.
I'll try to figure out what exactly is going wrong with my build a little later.
In the meantime it would be terrific if you could create the PR

[michaelklishin]

No worries, I had some head scratching with all the Xcode 13.3 changes myself just a week ago. I have a machine where the client builds already, now trying to see what instructions may be out of date on a "clean" one.

Then will run the tests and hopefully we can merge.

[michaelklishin]

Second attempt: #196

[michaelklishin]

Third attempt: #197

[Grimal]

One caveat I'm seeing. By making the heartbeatSender property weak, it isn't retained anywhere and thus is dropped. When start is called on the connection, strongThis.heartbeatSender will be nil. No heartbeats get sent, and the broker will kick the client.