Skip to content

Fix FIAM test flakes and exposed library bugs #3101

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 3, 2019
Merged

Fix FIAM test flakes and exposed library bugs #3101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3a880d0
Fix FIAM test flakes and resulting library bugs
paulb777 May 28, 2019
9dbd0c7
style
paulb777 May 28, 2019
00ac7c6
Fix naming of initWithExpireAdfterInSeconds:withTimeFetcher:withCache…
christibbs May 29, 2019
34ac17f
FIRIAMClearcutUploader: Only schedule next log send on init if there …
christibbs May 30, 2019
e7e3833
Fix whitespace
christibbs May 30, 2019
3b8991f
Merge branch 'master' into pb-fiam-flakes
christibbs May 30, 2019
0a88dc9
Make cachePath in `initWithExpireAfterInSeconds:withTimeFetcher:cache…
christibbs May 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Firebase/InAppMessaging/Analytics/FIRIAMClearcutLogStorage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ NS_ASSUME_NONNULL_BEGIN
// attempted.

@interface FIRIAMClearcutLogStorage : NSObject
- (instancetype)initWithExpireAfterInSeconds:(NSInteger)expireInSeconds
withTimeFetcher:(id<FIRIAMTimeFetcher>)timeFetcher
cachePath:(nullable NSString *)cachePath;

- (instancetype)initWithExpireAfterInSeconds:(NSInteger)expireInSeconds
withTimeFetcher:(id<FIRIAMTimeFetcher>)timeFetcher;

Expand Down
12 changes: 10 additions & 2 deletions Firebase/InAppMessaging/Analytics/FIRIAMClearcutLogStorage.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ + (NSString *)determineCacheFilePath {
}

- (instancetype)initWithExpireAfterInSeconds:(NSInteger)expireInSeconds
withTimeFetcher:(id<FIRIAMTimeFetcher>)timeFetcher {
withTimeFetcher:(id<FIRIAMTimeFetcher>)timeFetcher
cachePath:(nullable NSString *)cachePath {
if (self = [super init]) {
_records = [[NSMutableArray alloc] init];
_timeFetcher = timeFetcher;
Expand All @@ -92,7 +93,7 @@ - (instancetype)initWithExpireAfterInSeconds:(NSInteger)expireInSeconds
name:UIApplicationWillResignActiveNotification
object:nil];
@try {
[self loadFromCachePath:nil];
[self loadFromCachePath:cachePath];
} @catch (NSException *exception) {
FIRLogWarning(kFIRLoggerInAppMessaging, @"I-IAM230004",
@"Non-fatal exception in loading persisted clearcut log records: %@.",
Expand All @@ -102,6 +103,13 @@ - (instancetype)initWithExpireAfterInSeconds:(NSInteger)expireInSeconds
return self;
}

- (instancetype)initWithExpireAfterInSeconds:(NSInteger)expireInSeconds
withTimeFetcher:(id<FIRIAMTimeFetcher>)timeFetcher {
return [self initWithExpireAfterInSeconds:expireInSeconds
withTimeFetcher:timeFetcher
cachePath:nil];
}

- (void)appWillBecomeInactive {
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0ul), ^{
[self saveIntoCacheWithPath:nil];
Expand Down
27 changes: 15 additions & 12 deletions Firebase/InAppMessaging/Analytics/FIRIAMClearcutUploader.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,11 @@ - (instancetype)initWithRequestSender:(FIRIAMClearcutHttpRequestSender *)request
_nextValidSendTimeInMills = (int64_t)
[_userDefaults doubleForKey:FIRIAM_UserDefaultsKeyForNextValidClearcutUploadTimeInMills];

// seed the first send upon SDK start-up
[self scheduleNextSend];
Copy link
Member

@ryanwilson ryanwilson May 29, 2019

Choose a reason for hiding this comment

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

Will want to hear back from @christibbs about this one. We may want to do this in case there are actually pending logs that are written to disk?

Copy link
Contributor

@christibbs christibbs May 30, 2019

Choose a reason for hiding this comment

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

Trying out an approach where we only call this if there are pending logs to be sent.

NSArray<FIRIAMClearcutLogRecord *> *availableLogs =
[logStorage popStillValidRecordsForUpTo:strategy.batchSendSize];
if (availableLogs.count) {
[self scheduleNextSend];
}

FIRLogDebug(kFIRLoggerInAppMessaging, @"I-IAM260001",
@"FIRIAMClearcutUploader created with strategy as %@", self.strategy);
Expand All @@ -133,20 +136,20 @@ - (void)addNewLogRecord:(FIRIAMClearcutLogRecord *)record {
}

- (void)attemptUploading {
NSArray<FIRIAMClearcutLogRecord *> *availbleLogs =
NSArray<FIRIAMClearcutLogRecord *> *availableLogs =
[self.logStorage popStillValidRecordsForUpTo:self.strategy.batchSendSize];

if (availbleLogs.count > 0) {
if (availableLogs.count > 0) {
FIRLogDebug(kFIRLoggerInAppMessaging, @"I-IAM260011", @"Deliver %d clearcut records",
(int)availbleLogs.count);
(int)availableLogs.count);
[self.requestSender
sendClearcutHttpRequestForLogs:availbleLogs
sendClearcutHttpRequestForLogs:availableLogs
withCompletion:^(BOOL success, BOOL shouldRetryLogs,
int64_t waitTimeInMills) {
if (success) {
FIRLogDebug(kFIRLoggerInAppMessaging, @"I-IAM260003",
@"Delivering %d clearcut records was successful",
(int)availbleLogs.count);
(int)availableLogs.count);
// make sure the effective wait time is between two bounds
// defined in strategy
waitTimeInMills =
Expand All @@ -159,7 +162,7 @@ - (void)attemptUploading {
FIRLogDebug(kFIRLoggerInAppMessaging, @"I-IAM260004",
@"Failed to attempt the delivery of %d clearcut "
@"records and should-retry for them is %@",
(int)availbleLogs.count, shouldRetryLogs ? @"YES" : @"NO");
(int)availableLogs.count, shouldRetryLogs ? @"YES" : @"NO");
if (shouldRetryLogs) {
/**
* Note that there is a chance that the app crashes before we can
Expand All @@ -170,7 +173,7 @@ - (void)attemptUploading {
*/
FIRLogDebug(kFIRLoggerInAppMessaging, @"I-IAM260007",
@"Push failed log records back to storage");
[self.logStorage pushRecords:availbleLogs];
[self.logStorage pushRecords:availableLogs];
}

waitTimeInMills = (int64_t)self.strategy.failureBackoffTimeInMills;
Expand Down Expand Up @@ -207,10 +210,7 @@ - (void)attemptUploading {
- (void)scheduleNextSend {
@synchronized(self) {
if (_nextSendScheduled) {
// already scheduled next send, don't do it again
return;
} else {
_nextSendScheduled = YES;
}
}

Expand All @@ -228,6 +228,9 @@ - (void)scheduleNextSend {
_queue, ^{
[self attemptUploading];
});
@synchronized(self) {
_nextSendScheduled = YES;
}
}

@end
Loading