-
Notifications
You must be signed in to change notification settings - Fork 976
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
Reduce NSURLSession memory footprint #969
Conversation
Adds method to add default stripe headers to any request Setting the headers on requests instead of re-instantiating NSURLSession with new default headers avoids memory accumulation from the security framework (see https://developer.apple.com/library/archive/qa/qa1727/_index.html)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I think there're a couple changes left over from your investigation that can be removed.
Stripe/STPAPIClient.m
Outdated
@@ -160,7 +174,7 @@ - (void)setStripeAccount:(NSString *)stripeAccount { | |||
_stripeAccount = stripeAccount; | |||
|
|||
// Regenerate url session configuration | |||
self.urlSession = [NSURLSession sessionWithConfiguration:[self sessionConfiguration]]; | |||
// self.urlSession = [NSURLSession sessionWithConfiguration:[self sessionConfiguration]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be deleted, not commented out.
Stripe/STPAPIClient.m
Outdated
return [additionalHeaders copy]; | ||
} | ||
|
||
- (void)setUrlSession:(NSURLSession *)urlSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed/ever called? I saw you made the urlSession
readonly
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, this is a vestige of a previous experimient. Guess I forgot to clean up the branch before making this PR :P
@@ -63,7 +63,7 @@ - (void)testInitWithConfiguration { | |||
STPAPIClient *sut = [[STPAPIClient alloc] initWithConfiguration:config]; | |||
XCTAssertEqualObjects(sut.publishableKey, config.publishableKey); | |||
XCTAssertEqualObjects(sut.stripeAccount, config.stripeAccount); | |||
NSString *accountHeader = sut.urlSession.configuration.HTTPAdditionalHeaders[@"Stripe-Account"]; | |||
NSString *accountHeader = [sut configuredRequestForURL:[NSURL URLWithString:@"https://www.stripe.com"]].allHTTPHeaderFields[@"Stripe-Account"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Might be nice to extract this into a helper method on the test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Summary
Don't create a new NSURLSession instance for each ephemeral key request
Motivation
https://jira.corp.stripe.com/browse/IOS-773
Creating NSURLSessions induces a memory overhead because iOS allocates and saves cache space (ref. https://developer.apple.com/library/archive/qa/qa1727/_index.html). We were also not properly invalidating old sessions as we replaced them, aggravating the problem.
The other option which appears to remove all memory leaks is to use the shared instance of NSURLSession, but we want to keep a private instance. Using an ephemeral configuration (see link below) can also remove any of the reported links, but does not suit our use case.
More references:
https://stackoverflow.com/questions/42780244/swift-3-urlsession-memory-leak
https://stackoverflow.com/questions/28223345/memory-leak-when-using-nsurlsession-downloadtaskwithurl/35757989#35757989
http://footle.org/2015/10/10/fixing-a-swift-memory-leak/
Testing
I used the standard integration demo and was able to easily reproduce the reported leaks just by viewing an item. With the changes all leaks attributed to the Stripe library were gone.