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

Fixes memory leaks in IoT #1175

Closed

Conversation

jkennington
Copy link
Contributor

@jkennington jkennington commented Jan 14, 2019

Similar to earlier PR with another memory leak fixed and target most recent SDK version.

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.

The reconnectTimer was invalidated on whatever thread disconnect was called on rather than the reconnectThread causing a memory leak; this is fixed by having the reconnectTimer invalidated on the correct thread.

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

if (!self.runLoopShouldContinue ) {

if (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.

@cbommas, your earlier suggestion to simplify this was correct as this method will be called already on the correct thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@frankmuellr frankmuellr added iot Issues related to the IoT SDK pull request labels Jan 14, 2019
@frankmuellr frankmuellr requested a review from scb01 January 14, 2019 19:34
// NOTE: This does not work as intended. The reconnection attempt is always done on the reconnect thread
// but the timer needs to be invalidated on the streams thread to ensure memory is released.
// However, as currently implemented a switch to the streams thread here to clean this up is always beaten to the
// invalidation by other events which are are already being processed on the streams thread.
[self.connectionAgeTimer invalidate];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As noted this doesn't really work but other code avoids a memory leak. Ideally, this would have code to switch threads to clean up correctly but I left this alone for now as by the time a thread switch got control another event had already done the cleanup. I can update PR to have the thread switch if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this change, rather than doing a thread switch, lets just remove this code, as it is handled in one of the other events.

@scb01
Copy link
Contributor

scb01 commented Jan 28, 2019

@jkennington

Can we break this PR up into two separate ones? There are changes to the IOT code and the Core code.

The challenge I have with AWSCore code changes is that we had to make the change to not invalidate the session due to crashes ( see #913). We will have to dive deeper on this.

For the IOT changes, I will be testing them and prepping for release, pending the test results.

@jkennington
Copy link
Contributor Author

Have split into #1202 and #1203. Closing this PR.

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.

3 participants