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 crashes and memory leaks #4256

Merged
merged 11 commits into from Sep 24, 2018

Conversation

Projects
None yet
3 participants
@Kaspik
Contributor

Kaspik commented Jul 23, 2018

Good morning guys!

I have some improvements in AFNetowrking that I've done in November and was testing on our app since then. I would love to switch back to CocoaPods version from our fork so I'm opening PR for these Fabric crashes here.

  • invalidateSessionCancelingTasks should also invalidate session as per name - this means the session has to be able to be allocated again not only in init method - synchronized lazy loader looks like best solution
  • session in load can be taken from sharedSession - no reason to create new one by configuration
  • getTasksWithCompletionHandler has to use weak/strong

Verified on app with almost 2 mil. active users daily since November 2017.

Kaspik added some commits Nov 12, 2017

Merge branch 'master' into cd-master
* master: (71 commits)
  Specify Xcode 9.4 in Travis file.
  Update AFImageDownloader.m
  Safely get mergedTasks On responseQueue
  Update HTTPBin certificates.
  Update block usage requirements.
  Fixed tests.
  Update AFAutoPurgingImageCache.m
  Preparing for the 3.2.1 release
  Fix issues with AFCompatibilityMacros.h
  Add AFCompatibilityMacros.h to podspec.
  Add new .env files for iOS 11.x
  Update SDK settings in Travis config.
  Use old SDKs to test earlier versions of iOS 11.
  Update default SDK for iOS and tvOS
  Update umbrella header.
  Update HTTPBin certificates for April 2018.
  Remove beta from 9.3 image
  Update asset catalog JSON.
  Tweak tests to send expressions directly.
  Add tests and also prohibit +new from working.
  ...

# Conflicts:
#	AFNetworking/AFNetworkReachabilityManager.h
#	AFNetworking/AFURLSessionManager.m

@Kaspik Kaspik changed the title from Fix crashfixes and memory leaks to Fix crashes and memory leaks Jul 23, 2018

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Jul 23, 2018

Contributor

Hey @SlaunchaMan ! This PR has some fixes that are also opened as PRs separately. As I implemented them months ago and they are verified, it would be great to get them on master and to the next release. It closes multiple PRs opened here so I believe it helps you to clean to project as well.

Let me know how does it look, thanks!

Contributor

Kaspik commented Jul 23, 2018

Hey @SlaunchaMan ! This PR has some fixes that are also opened as PRs separately. As I implemented them months ago and they are verified, it would be great to get them on master and to the next release. It closes multiple PRs opened here so I believe it helps you to clean to project as well.

Let me know how does it look, thanks!

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Jul 23, 2018

Contributor

Test testRDAR17029580IsFixed is failing and I'm not sure why.
It's also green locally on my machine.
It's test for iOS 7 but Travis CI is not even running iOS 7 tests anymore.
Are you going to drop iOS 7 support? Should we still keep the tests there with these fixes?

@kcharwood / @nikitahils - As you implemented this fix / tests 4 years ago (#3205) - any ideas why mine changes would affect this test?

Contributor

Kaspik commented Jul 23, 2018

Test testRDAR17029580IsFixed is failing and I'm not sure why.
It's also green locally on my machine.
It's test for iOS 7 but Travis CI is not even running iOS 7 tests anymore.
Are you going to drop iOS 7 support? Should we still keep the tests there with these fixes?

@kcharwood / @nikitahils - As you implemented this fix / tests 4 years ago (#3205) - any ideas why mine changes would affect this test?

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Jul 24, 2018

Member

Can you explain more about the crashes you see when calling invalidateSessionCancelingTasks? A session manager manages a session; once that session is invalidated, I would expect the session manager would also need to be recreated.

Member

SlaunchaMan commented Jul 24, 2018

Can you explain more about the crashes you see when calling invalidateSessionCancelingTasks? A session manager manages a session; once that session is invalidated, I would expect the session manager would also need to be recreated.

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Jul 27, 2018

Contributor

@SlaunchaMan Specifically this change is memory leak fix, not crash fix.

The method invalidateSessionCancelingTasks is now also nilling the session as otherwise the session is not invalidated anytime at all and it's causing memory leak issue because session can then live even when it shouldn't.

We could probably leave the change only partially to change the session to lazy loader and nil it when it's called from other places in AFNetworking.
But in our case we tried and decided that the session can be invalidated anytime when there is nothing to do and all tasks are done and can be re-created when it's necessary to use it.

We are not seeing any issues since November 18th 2017 when we released our app with forked AFNetowrking and it also fixed a lot of crashes on CFDictionaryGetValue that has ben reported multiple times by multiple people here (like #4140).

As per your comment - session manager is managing the session. It can manage the session to be nil and then allocate it again, with similar configuration, but there is no need to create the manager itself again as the configuration of the manager would be the same and static.

Contributor

Kaspik commented Jul 27, 2018

@SlaunchaMan Specifically this change is memory leak fix, not crash fix.

The method invalidateSessionCancelingTasks is now also nilling the session as otherwise the session is not invalidated anytime at all and it's causing memory leak issue because session can then live even when it shouldn't.

We could probably leave the change only partially to change the session to lazy loader and nil it when it's called from other places in AFNetworking.
But in our case we tried and decided that the session can be invalidated anytime when there is nothing to do and all tasks are done and can be re-created when it's necessary to use it.

We are not seeing any issues since November 18th 2017 when we released our app with forked AFNetowrking and it also fixed a lot of crashes on CFDictionaryGetValue that has ben reported multiple times by multiple people here (like #4140).

As per your comment - session manager is managing the session. It can manage the session to be nil and then allocate it again, with similar configuration, but there is no need to create the manager itself again as the configuration of the manager would be the same and static.

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Jul 27, 2018

Member

Can you explain more about why you're calling invalidateSessionCancelingTasks? The way the API is described implies that there should be a 1:1 relationship between sessions and session managers; are you sure the leak is the session itself and not that you’re holding on to session managers for invalidated sessions?

Member

SlaunchaMan commented Jul 27, 2018

Can you explain more about why you're calling invalidateSessionCancelingTasks? The way the API is described implies that there should be a 1:1 relationship between sessions and session managers; are you sure the leak is the session itself and not that you’re holding on to session managers for invalidated sessions?

Kaspik added some commits Aug 13, 2018

Merge remote-tracking branch 'as_origin/master'
# Conflicts:
#	AFNetworking/AFNetworkReachabilityManager.h
#	AFNetworking/AFURLSessionManager.m
Merge branch 'master' into cd-master
# Conflicts:
#	Tests/Tests/AFURLSessionManagerTests.m
@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Aug 13, 2018

Contributor

@SlaunchaMan I did some research in our app and performed some tests. It looks like the invalidate could be removed. Currently releasing to production to see if it's true or not - I'll get back to you next week about situation.

Definitely looking forward to merge this as we switched for 2 weeks back to stable last AFNetworking version on CocoaPods and our crash rate increased quite a lot.

Contributor

Kaspik commented Aug 13, 2018

@SlaunchaMan I did some research in our app and performed some tests. It looks like the invalidate could be removed. Currently releasing to production to see if it's true or not - I'll get back to you next week about situation.

Definitely looking forward to merge this as we switched for 2 weeks back to stable last AFNetworking version on CocoaPods and our crash rate increased quite a lot.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 13, 2018

Codecov Report

Merging #4256 into master will decrease coverage by 0.17%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4256      +/-   ##
==========================================
- Coverage   88.21%   88.04%   -0.18%     
==========================================
  Files          46       46              
  Lines        5973     5970       -3     
==========================================
- Hits         5269     5256      -13     
- Misses        704      714      +10
Impacted Files Coverage Δ
Tests/Tests/AFURLSessionManagerTests.m 81.89% <100%> (-0.35%) ⬇️
AFNetworking/AFURLSessionManager.m 70.5% <75%> (-1.2%) ⬇️
AFNetworking/AFURLSessionManager.h 85.71% <0%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b892e56...3c54e1f. Read the comment docs.

codecov-io commented Aug 13, 2018

Codecov Report

Merging #4256 into master will decrease coverage by 0.17%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4256      +/-   ##
==========================================
- Coverage   88.21%   88.04%   -0.18%     
==========================================
  Files          46       46              
  Lines        5973     5970       -3     
==========================================
- Hits         5269     5256      -13     
- Misses        704      714      +10
Impacted Files Coverage Δ
Tests/Tests/AFURLSessionManagerTests.m 81.89% <100%> (-0.35%) ⬇️
AFNetworking/AFURLSessionManager.m 70.5% <75%> (-1.2%) ⬇️
AFNetworking/AFURLSessionManager.h 85.71% <0%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b892e56...3c54e1f. Read the comment docs.

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Aug 20, 2018

Contributor

@SlaunchaMan Morning! Getting back to you as I promised. So current setup of this PR looks good and is stable on our app. (2mil+ daily users)

Let me know what do you think about merging this so we can move forward ideally also with CocoaPods release.

Contributor

Kaspik commented Aug 20, 2018

@SlaunchaMan Morning! Getting back to you as I promised. So current setup of this PR looks good and is stable on our app. (2mil+ daily users)

Let me know what do you think about merging this so we can move forward ideally also with CocoaPods release.

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Aug 24, 2018

Contributor

@SlaunchaMan Jeff, you responding? What else can we do to fix these issues?

Contributor

Kaspik commented Aug 24, 2018

@SlaunchaMan Jeff, you responding? What else can we do to fix these issues?

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Aug 27, 2018

Member

@Kaspik Sorry, been busy with my day job. If you’re not invalidating sessions any more, do you still experience situations where you need to recreate the session in the session manager?

Member

SlaunchaMan commented Aug 27, 2018

@Kaspik Sorry, been busy with my day job. If you’re not invalidating sessions any more, do you still experience situations where you need to recreate the session in the session manager?

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Aug 28, 2018

Contributor

@SlaunchaMan No worries.

Yes. The invalidate method is still called sometimes - especially if app is backgrounded for longer time or anything else - and the session can be then cleared out and created again with fresh config. Reverting back to not setting it to nil also bring back some issues on Fabric... 😒

Contributor

Kaspik commented Aug 28, 2018

@SlaunchaMan No worries.

Yes. The invalidate method is still called sometimes - especially if app is backgrounded for longer time or anything else - and the session can be then cleared out and created again with fresh config. Reverting back to not setting it to nil also bring back some issues on Fabric... 😒

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Aug 28, 2018

Member

@Kaspik That makes sense, but I’m wondering why you can’t simply recreate the session manager instead of recreating its session.

Member

SlaunchaMan commented Aug 28, 2018

@Kaspik That makes sense, but I’m wondering why you can’t simply recreate the session manager instead of recreating its session.

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Aug 30, 2018

Contributor

@SlaunchaMan Is there any reason for that? The manager has still the same settings and it would do the same job just with more work. Now the manager is really managing session and is taking care of that for us - not you as a person by managing the manager.

Right now it handles it self + the session where session can be invalidated (as per by name of the function) and then recreated when needed by lazy loader.

Contributor

Kaspik commented Aug 30, 2018

@SlaunchaMan Is there any reason for that? The manager has still the same settings and it would do the same job just with more work. Now the manager is really managing session and is taking care of that for us - not you as a person by managing the manager.

Right now it handles it self + the session where session can be invalidated (as per by name of the function) and then recreated when needed by lazy loader.

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Sep 5, 2018

Contributor

Any update on merging this? If not, I'm going to switch back to our fork because crashes happening on current AFNetworking library are pretty bad (as you can see, it started when we switched back to no-fork version) - http://crashes.to/s/6daea4ae313

Contributor

Kaspik commented Sep 5, 2018

Any update on merging this? If not, I'm going to switch back to our fork because crashes happening on current AFNetworking library are pretty bad (as you can see, it started when we switched back to no-fork version) - http://crashes.to/s/6daea4ae313

@SlaunchaMan

This comment has been minimized.

Show comment
Hide comment
@SlaunchaMan

SlaunchaMan Sep 8, 2018

Member

@Kaspik I think it’ll be OK to make this change. We should try to restrict this PR to making that one change—that is, allowing a session manager to recreate its session. I’ll make some more PR comments to that effect to remove some of the other stuff. Can you modify the invalidateSessionCancelingTasks method to take another BOOL parameter, something like recreateSession, and have that control whether or not the session is recreated? The implementation of invalidateSessionCancelingTasks would then call this new method with that parameter set to NO, so the behavior won’t change for existing code.

Member

SlaunchaMan commented Sep 8, 2018

@Kaspik I think it’ll be OK to make this change. We should try to restrict this PR to making that one change—that is, allowing a session manager to recreate its session. I’ll make some more PR comments to that effect to remove some of the other stuff. Can you modify the invalidateSessionCancelingTasks method to take another BOOL parameter, something like recreateSession, and have that control whether or not the session is recreated? The implementation of invalidateSessionCancelingTasks would then call this new method with that parameter set to NO, so the behavior won’t change for existing code.

@SlaunchaMan

With these changes and my previous comment, I think we’d be good to go for the next release!

Show outdated Hide outdated AFNetworking/AFURLSessionManager.m Outdated
Show outdated Hide outdated AFNetworking/AFURLSessionManager.m Outdated
@@ -556,6 +555,19 @@ - (void)dealloc {
#pragma mark -
- (NSURLSession *)session {

This comment has been minimized.

@SlaunchaMan

SlaunchaMan Sep 8, 2018

Member

Instead of making a new getter that automatically creates the session, let’s make a new intiializeSession method that creates the session with the saved session configuration and delegate queue, and call it from the initializer.

@SlaunchaMan

SlaunchaMan Sep 8, 2018

Member

Instead of making a new getter that automatically creates the session, let’s make a new intiializeSession method that creates the session with the saved session configuration and delegate queue, and call it from the initializer.

This comment has been minimized.

@Kaspik

Kaspik Sep 11, 2018

Contributor

hmmm, let's think about this one. I can change the setter in invalidateSessionCancelingTasks to nil session only if user wants, but I would love to leave the lazy loader here for two reasons:

  1. Lazy loader makes sure that if we nil the session, it get's recreated again and the behavior doesn't change
  2. it fixes bug on iOS that you mentioned below - let's take a look on the test: // iOS 7 has a bug that may return nil for a session. To simulate that, nil out the session and it will return nil itself - this is wrong approach I think. If iOS 7 has bug where it can return nil, we should fix this bug by having lazy loader and not have the session nil at all - ever. So this is like 2 in 1 fix of initialize method + iOS 7 bug fix. What do you think?
@Kaspik

Kaspik Sep 11, 2018

Contributor

hmmm, let's think about this one. I can change the setter in invalidateSessionCancelingTasks to nil session only if user wants, but I would love to leave the lazy loader here for two reasons:

  1. Lazy loader makes sure that if we nil the session, it get's recreated again and the behavior doesn't change
  2. it fixes bug on iOS that you mentioned below - let's take a look on the test: // iOS 7 has a bug that may return nil for a session. To simulate that, nil out the session and it will return nil itself - this is wrong approach I think. If iOS 7 has bug where it can return nil, we should fix this bug by having lazy loader and not have the session nil at all - ever. So this is like 2 in 1 fix of initialize method + iOS 7 bug fix. What do you think?

This comment has been minimized.

@SlaunchaMan

SlaunchaMan Sep 11, 2018

Member

Yeah, actually that would work. Since the default, current behavior wouldn’t nil out the session, the lazy loader wouldn’t affect existing users. Then we could clean up all of that documentation about the iOS 7 bug!

@SlaunchaMan

SlaunchaMan Sep 11, 2018

Member

Yeah, actually that would work. Since the default, current behavior wouldn’t nil out the session, the lazy loader wouldn’t affect existing users. Then we could clean up all of that documentation about the iOS 7 bug!

Show outdated Hide outdated AFNetworking/AFURLSessionManager.m Outdated
Show outdated Hide outdated Tests/Tests/AFURLSessionManagerTests.m Outdated
@@ -533,17 +529,20 @@ - (instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration *)config
self.lock = [[NSLock alloc] init];
self.lock.name = AFURLSessionManagerLockName;
__weak typeof(self) weakSelf = self;

This comment has been minimized.

@SlaunchaMan

SlaunchaMan Sep 8, 2018

Member

This is likely unnecessary; the getTasksWithCompletionHandler method is short-lived, and we probably want to keep the session manager around until it ends.

@SlaunchaMan

SlaunchaMan Sep 8, 2018

Member

This is likely unnecessary; the getTasksWithCompletionHandler method is short-lived, and we probably want to keep the session manager around until it ends.

This comment has been minimized.

@Kaspik

Kaspik Sep 11, 2018

Contributor

Because we are then assigning it to __strong, it explicitly holds the manager around until it ends (if we would use only __weak, then you are right and we would lose the manager). Without this change - self is deallocated during the enumeration and memory leaks appear.

@Kaspik

Kaspik Sep 11, 2018

Contributor

Because we are then assigning it to __strong, it explicitly holds the manager around until it ends (if we would use only __weak, then you are right and we would lose the manager). Without this change - self is deallocated during the enumeration and memory leaks appear.

This comment has been minimized.

@SlaunchaMan

SlaunchaMan Sep 11, 2018

Member

Right, I mean we could just use self inside the completion handler without fear. The completion handler is only going to maintain a strong reference to self while it’s executing; once it’s done, then that reference goes away. So we don’t need to do the __weak/__strong dance because this isn’t a retain cycle.

@SlaunchaMan

SlaunchaMan Sep 11, 2018

Member

Right, I mean we could just use self inside the completion handler without fear. The completion handler is only going to maintain a strong reference to self while it’s executing; once it’s done, then that reference goes away. So we don’t need to do the __weak/__strong dance because this isn’t a retain cycle.

This comment has been minimized.

@Kaspik

Kaspik Sep 11, 2018

Contributor

mmm, I still think there is problem with keeping self inside the closure and there is retain cycle. If it doesn't hurt, I would love to leave it there. :)

@Kaspik

Kaspik Sep 11, 2018

Contributor

mmm, I still think there is problem with keeping self inside the closure and there is retain cycle. If it doesn't hurt, I would love to leave it there. :)

@SlaunchaMan SlaunchaMan added this to the 3.3.0 milestone Sep 8, 2018

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Sep 11, 2018

Contributor

@SlaunchaMan Responded, updated, added more tests, updated existing ones!

Contributor

Kaspik commented Sep 11, 2018

@SlaunchaMan Responded, updated, added more tests, updated existing ones!

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Sep 11, 2018

Contributor

screenshot 2018-09-11 at 13 27 59
Okay, I have no idea what's going on there on Travis, but tests succeeded locally and I'm not gonna be fixing something that is not broken at all. 😄

Contributor

Kaspik commented Sep 11, 2018

screenshot 2018-09-11 at 13 27 59
Okay, I have no idea what's going on there on Travis, but tests succeeded locally and I'm not gonna be fixing something that is not broken at all. 😄

@Kaspik

This comment has been minimized.

Show comment
Hide comment
@Kaspik

Kaspik Sep 11, 2018

Contributor

@SlaunchaMan Okay, what are we gonna do about the tests to finish this up? I think we handled all comments.

Contributor

Kaspik commented Sep 11, 2018

@SlaunchaMan Okay, what are we gonna do about the tests to finish this up? I think we handled all comments.

@SlaunchaMan SlaunchaMan merged commit 1bd73c7 into AFNetworking:master Sep 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment