Skip to content
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

Fix memory leaks for IoT #1037

Closed

Conversation

jkennington
Copy link
Contributor

@jkennington jkennington commented Sep 11, 2018

After removing an AWSIoTDataManager, the AWSIoTDataManager, AWSIoTMQTTClient, AWSURLSessionManager and other related entities were leaked.

This fixes a retain cycle between the AWSIoTMQTTClient where it held a strong reference to the associatedObject the AWSIoTDataManager which was holding a strong reference to the mqttClient. This makes the associatedObject reference weak.

If an AWSIoTDataManager was disconnected before the connectionAgeInSeconds exceeded the minimumConnectionTime the connectionAgeTimer was never invalidated and held a strong reference to the AWSIoTMQTTClient (e.g. self passed to the timer on creation of the timer). This fixes the issue by issuing an invalidate to the timer on the correct thread in the event the user requests a disconnect before reaching the minimumConnectionTime.

The AWSURLSessionManager creates and passes self to an NSURLSession but never invalidated the session to release the strong reference to self. AWSNetworking could be released but the AWSURLSessionManager leaked being held by the NSURLSession. This is fixed by having the AWSNetworking let the AWSURLSessionManager know it needs to invalidate the session when it is being released, as none of them should be needed after the AWSNetworking creating them is released.

@scb01 scb01 self-assigned this Sep 11, 2018
@scb01 scb01 added the iot Issues related to the IoT SDK label Sep 28, 2018
@@ -99,7 +99,7 @@
/**
An optional associated object (nil by default).
*/
@property(nonatomic, strong) NSObject *associatedObject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this change.

@@ -639,6 +639,16 @@ - (void)openStreams:(id)sender
[defaultRunLoopTimer invalidate];

if (!self.runLoopShouldContinue ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The connectionAgeTimer will self terminate once the max age has been reached, but if a disconnect is issued before then, it will continue to run and hold on to the object reference.

I think we can simplify this by just doing this after line 639. Thoughts?

     if (self.connectionAgeTimer != nil ) {
            [self.connectionAgeTimer invalidate];
            self.connectionAgeTimer = nil;
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we need to move the invalidate call because of the documentation below.

From https://developer.apple.com/documentation/foundation/nstimer/1415405-invalidate?language=objc

Special Considerations
You must send this message from the thread on which the timer was installed. If you send this message from another thread, the input source associated with the timer may not be removed from its run loop, which could prevent the thread from exiting properly.

@jkennington
Copy link
Contributor Author

Updated PR at #1175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iot Issues related to the IoT SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants