-
Notifications
You must be signed in to change notification settings - Fork 294
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
Update Configuration Threading Logic #807
Changes from all commits
52af920
d458b41
debcf4b
0dba5e6
7bfdf7e
4e57a00
f8daa4d
c05ae76
f1f88af
b71ee40
573c590
ed2c160
78884de
c24079a
97ea7b8
cda935b
26a4694
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,7 +90,8 @@ - (nullable instancetype)initWithAuthorization:(NSString *)authorization sendAna | |
configurationCache = [[NSURLCache alloc] initWithMemoryCapacity:1 * 1024 * 1024 diskCapacity:0 diskPath:nil]; | ||
}); | ||
configuration.URLCache = configurationCache; | ||
configuration.requestCachePolicy = NSURLRequestReturnCacheDataElseLoad; | ||
// Use the caching logic defined in the protocol implementation, if any, for a particular URL load request. | ||
configuration.requestCachePolicy = NSURLRequestUseProtocolCachePolicy; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. random q: does config endpoint enforce caching with HTTP headers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean like passing a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right yeah I guess that's a question for the gateway actually. If config has caching behavior built in we could leverage that on Android too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah certainly something to look into for both SDKs |
||
_configurationHTTP.session = [NSURLSession sessionWithConfiguration:configuration]; | ||
|
||
// Kickoff the background request to fetch the config | ||
|
@@ -273,75 +274,56 @@ - (void)fetchPaymentMethodNonces:(BOOL)defaultFirst completion:(void (^)(NSArray | |
#pragma mark - Remote Configuration | ||
|
||
- (void)fetchOrReturnRemoteConfiguration:(void (^)(BTConfiguration *, NSError *))completionBlock { | ||
// Guarantee that multiple calls to this method will successfully obtain configuration exactly once. | ||
// Fetches or returns the configuration and caches the response in the GET BTHTTP call if successful | ||
// | ||
// Rules: | ||
// - If cachedConfiguration is present, return it without a request | ||
// - If cachedConfiguration is not present, fetch it and cache the succesful response | ||
// - If fetching fails, return error and the next queued will try to fetch again | ||
// | ||
// Note: Configuration queue is SERIAL. This helps ensure that each request for configuration | ||
// is processed independently. Thus, the check for cached configuration and the fetch is an | ||
// atomic operation with respect to other calls to this method. | ||
// | ||
// Note: Uses dispatch_semaphore to block the configuration queue when the configuration fetch | ||
// request is waiting to return. In this context, it is OK to block, as the configuration | ||
// queue is a background queue to guarantee atomic access to the remote configuration resource. | ||
dispatch_async(self.configurationQueue, ^{ | ||
__block NSError *fetchError; | ||
|
||
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); | ||
jaxdesmarais marked this conversation as resolved.
Show resolved
Hide resolved
|
||
__block BTConfiguration *configuration; | ||
NSString *configPath = @"v1/configuration"; // Default for tokenizationKey | ||
if (self.clientToken) { | ||
configPath = [self.clientToken.configURL absoluteString]; | ||
} | ||
[self.configurationHTTP GET:configPath parameters:@{ @"configVersion": @"3" } shouldCache:YES completion:^(BTJSON * _Nullable body, NSHTTPURLResponse * _Nullable response, NSError * _Nullable error) { | ||
if (error) { | ||
fetchError = error; | ||
} else if (response.statusCode != 200) { | ||
NSError *configurationDomainError = | ||
[NSError errorWithDomain:BTAPIClientErrorDomain | ||
code:BTAPIClientErrorTypeConfigurationUnavailable | ||
userInfo:@{ | ||
NSLocalizedFailureReasonErrorKey: @"Unable to fetch remote configuration from Braintree API at this time." | ||
}]; | ||
fetchError = configurationDomainError; | ||
} else { | ||
configuration = [[BTConfiguration alloc] initWithJSON:body]; | ||
if (!self.braintreeAPI) { | ||
NSURL *apiURL = [configuration.json[@"braintreeApi"][@"url"] asURL]; | ||
NSString *accessToken = [configuration.json[@"braintreeApi"][@"accessToken"] asString]; | ||
self.braintreeAPI = [[BTAPIHTTP alloc] initWithBaseURL:apiURL accessToken:accessToken]; | ||
} | ||
if (!self.http) { | ||
NSURL *baseURL = [configuration.json[@"clientApiUrl"] asURL]; | ||
if (self.clientToken) { | ||
self.http = [[BTHTTP alloc] initWithBaseURL:baseURL authorizationFingerprint:self.clientToken.authorizationFingerprint]; | ||
} else if (self.tokenizationKey) { | ||
self.http = [[BTHTTP alloc] initWithBaseURL:baseURL tokenizationKey:self.tokenizationKey]; | ||
} | ||
// - If cachedConfiguration is not present, fetch it and cache the successful response | ||
// - If fetching fails, return error | ||
NSString *configPath = @"v1/configuration"; // Default for tokenizationKey | ||
if (self.clientToken) { | ||
configPath = [self.clientToken.configURL absoluteString]; | ||
} | ||
[self.configurationHTTP GET:configPath parameters:@{ @"configVersion": @"3" } shouldCache:YES completion:^(BTJSON * _Nullable body, NSHTTPURLResponse * _Nullable response, NSError * _Nullable error) { | ||
NSError *fetchError; | ||
BTConfiguration *configuration; | ||
|
||
if (error) { | ||
fetchError = error; | ||
} else if (response.statusCode != 200) { | ||
NSError *configurationDomainError = | ||
[NSError errorWithDomain:BTAPIClientErrorDomain | ||
code:BTAPIClientErrorTypeConfigurationUnavailable | ||
userInfo:@{ | ||
NSLocalizedFailureReasonErrorKey: @"Unable to fetch remote configuration from Braintree API at this time." | ||
}]; | ||
fetchError = configurationDomainError; | ||
} else { | ||
configuration = [[BTConfiguration alloc] initWithJSON:body]; | ||
if (!self.braintreeAPI) { | ||
NSURL *apiURL = [configuration.json[@"braintreeApi"][@"url"] asURL]; | ||
NSString *accessToken = [configuration.json[@"braintreeApi"][@"accessToken"] asString]; | ||
self.braintreeAPI = [[BTAPIHTTP alloc] initWithBaseURL:apiURL accessToken:accessToken]; | ||
} | ||
if (!self.http) { | ||
NSURL *baseURL = [configuration.json[@"clientApiUrl"] asURL]; | ||
if (self.clientToken) { | ||
self.http = [[BTHTTP alloc] initWithBaseURL:baseURL authorizationFingerprint:self.clientToken.authorizationFingerprint]; | ||
} else if (self.tokenizationKey) { | ||
self.http = [[BTHTTP alloc] initWithBaseURL:baseURL tokenizationKey:self.tokenizationKey]; | ||
} | ||
if (!self.graphQL) { | ||
NSURL *graphQLBaseURL = [BTAPIClient graphQLURLForEnvironment:configuration.environment]; | ||
if (self.clientToken) { | ||
self.graphQL = [[BTGraphQLHTTP alloc] initWithBaseURL:graphQLBaseURL authorizationFingerprint:self.clientToken.authorizationFingerprint]; | ||
} else if (self.tokenizationKey) { | ||
self.graphQL = [[BTGraphQLHTTP alloc] initWithBaseURL:graphQLBaseURL tokenizationKey:self.tokenizationKey]; | ||
} | ||
} | ||
if (!self.graphQL) { | ||
NSURL *graphQLBaseURL = [BTAPIClient graphQLURLForEnvironment:configuration.environment]; | ||
if (self.clientToken) { | ||
self.graphQL = [[BTGraphQLHTTP alloc] initWithBaseURL:graphQLBaseURL authorizationFingerprint:self.clientToken.authorizationFingerprint]; | ||
} else if (self.tokenizationKey) { | ||
self.graphQL = [[BTGraphQLHTTP alloc] initWithBaseURL:graphQLBaseURL tokenizationKey:self.tokenizationKey]; | ||
} | ||
} | ||
|
||
// Important: Unlock semaphore in all cases | ||
dispatch_semaphore_signal(semaphore); | ||
}]; | ||
|
||
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER); | ||
|
||
dispatch_async(dispatch_get_main_queue(), ^{ | ||
completionBlock(configuration, fetchError); | ||
}); | ||
}); | ||
} | ||
completionBlock(configuration, fetchError); | ||
}]; | ||
} | ||
|
||
#pragma mark - Analytics | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,7 +129,6 @@ - (instancetype)initWithAPIClient:(BTAPIClient *)apiClient { | |
_sessionsQueue = dispatch_queue_create("com.braintreepayments.BTAnalyticsService", DISPATCH_QUEUE_SERIAL); | ||
_apiClient = apiClient; | ||
_flushThreshold = 1; | ||
[[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(appWillResign:) name:UIApplicationWillResignActiveNotification object:nil]; | ||
} | ||
return self; | ||
} | ||
|
@@ -241,27 +240,6 @@ - (void)flush:(void (^)(NSError *))completionBlock { | |
}]; | ||
} | ||
|
||
#pragma mark - Private methods | ||
|
||
- (void)appWillResign:(NSNotification *)notification { | ||
UIApplication *application = notification.object; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. qq: Are analytics all processed in the foreground now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We didn't change the thread where analytics are being processed which is done in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah background tasks do still run when the app is closed by the user, it may be good to do this in the future. On Android we're using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, previously we were getting a ton of flush analytics errors which this resolves. I think we should certainly look into something similar in the future. When we re-write the core module in Swift I think there is certainly a lot of optimization to be done as we move to newer APIs with those changes in the future. |
||
|
||
__block UIBackgroundTaskIdentifier bgTask; | ||
bgTask = [application beginBackgroundTaskWithName:@"BTAnalyticsService" expirationHandler:^{ | ||
[[BTLogger sharedLogger] warning:@"Analytics service background task expired"]; | ||
[application endBackgroundTask:bgTask]; | ||
bgTask = UIBackgroundTaskInvalid; | ||
}]; | ||
|
||
// Start the long-running task and return immediately. | ||
dispatch_async(self.sessionsQueue, ^{ | ||
[self flush:^(__unused NSError * _Nullable error) { | ||
[application endBackgroundTask:bgTask]; | ||
bgTask = UIBackgroundTaskInvalid; | ||
}]; | ||
}); | ||
} | ||
|
||
#pragma mark - Helpers | ||
|
||
- (void)enqueueEvent:(NSString *)eventKind { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,15 +156,19 @@ - (void)httpRequestWithCaching:(NSString *)method path:(NSString *)aPath paramet | |
[[NSURLCache sharedURLCache] removeAllCachedResponses]; | ||
cachedResponse = nil; | ||
} | ||
|
||
if (cachedResponse != nil) { | ||
[self handleRequestCompletion:cachedResponse.data request:nil shouldCache:NO response:cachedResponse.response error:nil completionBlock:completionBlock]; | ||
} else { | ||
NSURLSessionTask *task = [self.session dataTaskWithRequest:request completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) { | ||
[self handleRequestCompletion:data request:request shouldCache:YES response:response error:error completionBlock:completionBlock]; | ||
}]; | ||
[task resume]; | ||
} | ||
|
||
// The increase in speed of API calls with cached configuration caused an increase in "network connection lost" errors. | ||
// Adding this delay allows us to throttle the network requests slightly to reduce load on the servers and decrease connection lost errors. | ||
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0.1 * NSEC_PER_SEC), dispatch_get_main_queue(), ^{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to put the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm.. we probably shouldn't change this value without the same amount of testing we did initially (we also tested smaller increments and this seemed to be the shortest amount of time that improved the issue). It's also not used elsewhere so I think leaving it here is ok, but happy to change it if others disagree |
||
if (cachedResponse != nil) { | ||
[self handleRequestCompletion:cachedResponse.data request:nil shouldCache:NO response:cachedResponse.response error:nil completionBlock:completionBlock]; | ||
} else { | ||
NSURLSessionTask *task = [self.session dataTaskWithRequest:request completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) { | ||
[self handleRequestCompletion:data request:request shouldCache:YES response:response error:error completionBlock:completionBlock]; | ||
}]; | ||
[task resume]; | ||
} | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! We only want to cache configuration requests (not anything else) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the overall solution to prevent multiple BT API requests within a 1 second timeframe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, essentially we are throttling this request by 0.1 seconds There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh aight Analytics HTTP requests happen as soon as they're triggered on iOS that is rough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't mean to nit I genuinely have questions lol. Did we choose 0.1 because it worked best in testing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah we tried 0.05 and 0.075 but we still saw an increased number of Network connection lost errors. 0.1 seems to be the least amount of time that still improved the errors. |
||
}]; | ||
} | ||
|
||
|
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.
This UI test if only used to test that the SDK successfully switches to the mock Venmo app and returns. Since we updated the demo app to include vaulting, this flow now ultimately fails, because the mock Venmo app does not return a valid nonce for vaulting. We decided to just update this error message, so that we can keep vaulting in the demo app flow for future testing, and since this test still covers its intended scope.