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 from missing invalidations #1203

Conversation

jkennington
Copy link
Contributor

@jkennington jkennington commented Jan 29, 2019

Core framework side of fixes needed to avoid IoT memory leaks.

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.

This will still need work to avoid problem due to existing bugs in other consumption of the Core framework.

From @cbommas

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.

These changes, in conjunction with #1202, fix leaking IoT objects.

@scb01 scb01 self-requested a review January 29, 2019 16:42
@scb01 scb01 self-assigned this Jan 29, 2019
@frankmuellr frankmuellr added the iot Issues related to the IoT SDK label Feb 8, 2019
@scb01 scb01 added core Issues related to AWSCore and removed iot Issues related to the IoT SDK labels Feb 23, 2019
@palpatim palpatim requested review from palpatim and removed request for scb01 July 11, 2019 21:08
@palpatim palpatim changed the base branch from master to develop July 11, 2019 21:09
@palpatim
Copy link
Member

palpatim commented Jul 11, 2019

Looking at this in relation to #913, I'm concerned that we may our retry logic may attempt to reissue a request after the session has been invalidated. I'm exploring this more to see if I can prove or disprove this.

So far, I've done the following locally:

  • Used finishTasksAndInvalidate rather than invalidateAndCancel. Since this is a core piece of the SDK, I'm uncomfortable with killing tasks mid-flight; we may have some that are intentionally fire-and-forget, but should be completed even though they don't need a callback.
  • Added the - [(void)URLSession:(NSURLSession *)session didBecomeInvalidWithError:(NSError *)error] NSURLSessionDelegate method on AWSURLSessionManager, which I am using to nil out the session with the intent of preventing any further activity on it.

I'm also considering adding some kind of isInvalidated flag, to guard against adding new tasks to the soon-to-be invalidated session.

I'll update this when I have more info.

@palpatim
Copy link
Member

palpatim commented Aug 9, 2019

@jkennington

I have submitted PR #1728 based on this original PR. In it, I made a number of changes to your original approach that I think will make this a bit safer and mitigate the risk of accidentally reintroducing the crash reported in #913, which was what led to the removal in the first place.

I am closing this PR in favor of that one, but thank you for the initial work getting this underway.

@palpatim palpatim closed this Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues related to AWSCore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants