From d372613260e3974e29cc98fe471e78c41b3d954c Mon Sep 17 00:00:00 2001 From: Themis wang Date: Tue, 4 Jun 2024 13:08:58 -0400 Subject: [PATCH 1/3] make sure rollouts logging queue is not nil --- .../FIRCLSRolloutsPersistenceManager.h | 3 +- .../FIRCLSRolloutsPersistenceManager.m | 37 ++++++++++++------- Crashlytics/Crashlytics/FIRCrashlytics.m | 6 ++- .../FIRCLSRolloutsPersistenceManagerTests.m | 18 ++++++--- 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.h b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.h index b9b2dd622fb..adbac49da2f 100644 --- a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.h +++ b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.h @@ -25,7 +25,8 @@ @interface FIRCLSRolloutsPersistenceManager : NSObject -- (instancetype _Nullable)initWithFileManager:(FIRCLSFileManager *_Nonnull)fileManager; +- (instancetype _Nullable)initWithFileManager:(FIRCLSFileManager *_Nonnull)fileManager + andQueue:(dispatch_queue_t _Nonnull)queue; - (instancetype _Nonnull)init NS_UNAVAILABLE; + (instancetype _Nonnull)new NS_UNAVAILABLE; diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m index 57993a965f3..9f7233b0297 100644 --- a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m +++ b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m @@ -13,7 +13,6 @@ // limitations under the License. #import -#include "Crashlytics/Crashlytics/Components/FIRCLSGlobals.h" #include "Crashlytics/Crashlytics/Components/FIRCLSUserLogging.h" #import "Crashlytics/Crashlytics/Helpers/FIRCLSLogger.h" #import "Crashlytics/Crashlytics/Models/FIRCLSFileManager.h" @@ -32,15 +31,23 @@ @interface FIRCLSRolloutsPersistenceManager : NSObject @property(nonatomic, readonly) FIRCLSFileManager *fileManager; +@property(nonnull, nonatomic, readonly) dispatch_queue_t rolloutsLoggingQueue; @end @implementation FIRCLSRolloutsPersistenceManager -- (instancetype)initWithFileManager:(FIRCLSFileManager *)fileManager { +- (instancetype)initWithFileManager:(FIRCLSFileManager *)fileManager + andQueue:(dispatch_queue_t)queue { self = [super init]; if (!self) { return nil; } _fileManager = fileManager; + + if (!queue) { + FIRCLSDebugLog(@"Failed to intialize FIRCLSRolloutsPersistenceManager, logging queue is nil"); + return nil; + } + _rolloutsLoggingQueue = queue; return self; } @@ -57,18 +64,20 @@ - (void)updateRolloutsStateToPersistenceWithRollouts:(NSData *_Nonnull)rollouts NSFileHandle *rolloutsFile = [NSFileHandle fileHandleForUpdatingAtPath:rolloutsPath]; - dispatch_async(FIRCLSGetLoggingQueue(), ^{ - @try { - [rolloutsFile seekToEndOfFile]; - NSMutableData *rolloutsWithNewLineData = [rollouts mutableCopy]; - [rolloutsWithNewLineData appendData:[@"\n" dataUsingEncoding:NSUTF8StringEncoding]]; - [rolloutsFile writeData:rolloutsWithNewLineData]; - [rolloutsFile closeFile]; - } @catch (NSException *exception) { - FIRCLSDebugLog(@"Failed to write new rollouts. Exception name: %s - message: %s", - exception.name, exception.reason); - } - }); + if (_rolloutsLoggingQueue) { + dispatch_async(_rolloutsLoggingQueue, ^{ + @try { + [rolloutsFile seekToEndOfFile]; + NSMutableData *rolloutsWithNewLineData = [rollouts mutableCopy]; + [rolloutsWithNewLineData appendData:[@"\n" dataUsingEncoding:NSUTF8StringEncoding]]; + [rolloutsFile writeData:rolloutsWithNewLineData]; + [rolloutsFile closeFile]; + } @catch (NSException *exception) { + FIRCLSDebugLog(@"Failed to write new rollouts. Exception name: %s - message: %s", + exception.name, exception.reason); + } + }); + } } - (void)debugLogWithMessage:(NSString *_Nonnull)message { diff --git a/Crashlytics/Crashlytics/FIRCrashlytics.m b/Crashlytics/Crashlytics/FIRCrashlytics.m index ab6c353e3da..ee1f6f2bd66 100644 --- a/Crashlytics/Crashlytics/FIRCrashlytics.m +++ b/Crashlytics/Crashlytics/FIRCrashlytics.m @@ -211,7 +211,11 @@ - (instancetype)initWithApp:(FIRApp *)app FIRCLSDebugLog(@"Registering RemoteConfig SDK subscription for rollouts data"); FIRCLSRolloutsPersistenceManager *persistenceManager = - [[FIRCLSRolloutsPersistenceManager alloc] initWithFileManager:_fileManager]; + [[FIRCLSRolloutsPersistenceManager alloc] + initWithFileManager:_fileManager + andQueue:dispatch_queue_create( + "com.google.firebase.FIRCLSRolloutsPersistence", + DISPATCH_QUEUE_SERIAL)]; _remoteConfigManager = [[FIRCLSRemoteConfigManager alloc] initWithRemoteConfig:remoteConfig persistenceDelegate:persistenceManager]; diff --git a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m index 4caceb242b1..9c0dbe198bd 100644 --- a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m @@ -30,6 +30,7 @@ @interface FIRCLSRolloutsPersistenceManagerTests : XCTestCase @property(nonatomic, strong) FIRCLSTempMockFileManager *fileManager; +@property(nonatomic, strong) dispatch_queue_t loggingQueue; @property(nonatomic, strong) FIRCLSRolloutsPersistenceManager *rolloutsPersistenceManager; @end @@ -41,8 +42,11 @@ - (void)setUp { [self.fileManager createReportDirectories]; [self.fileManager setupNewPathForExecutionIdentifier:reportId]; + self.loggingQueue = + dispatch_queue_create("com.google.firebase.FIRCLSRolloutsPersistence", DISPATCH_QUEUE_SERIAL); self.rolloutsPersistenceManager = - [[FIRCLSRolloutsPersistenceManager alloc] initWithFileManager:self.fileManager]; + [[FIRCLSRolloutsPersistenceManager alloc] initWithFileManager:self.fileManager + andQueue:self.loggingQueue]; } - (void)tearDown { @@ -65,18 +69,22 @@ - (void)testUpdateRolloutsStateToPersistenceWithRollouts { NSString *rolloutsFilePath = [[[self.fileManager activePath] stringByAppendingPathComponent:reportId] stringByAppendingPathComponent:FIRCLSReportRolloutsFile]; - [self.rolloutsPersistenceManager updateRolloutsStateToPersistenceWithRollouts:data reportID:reportId]; - + XCTAssertNotNil(self.loggingQueue); // Wait for the logging queue to finish. - dispatch_async(FIRCLSGetLoggingQueue(), ^{ + dispatch_async(self.loggingQueue, ^{ [expectation fulfill]; }); [self waitForExpectations:@[ expectation ] timeout:3]; XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath:rolloutsFilePath]); + + NSFileHandle *rolloutsFile = [NSFileHandle fileHandleForUpdatingAtPath:rolloutsFilePath]; + NSData *fileData = [rolloutsFile readDataToEndOfFile]; + NSString *fileString = [[NSString alloc] initWithData:fileData encoding:NSUTF8StringEncoding]; + XCTAssertTrue([fileString isEqualToString:[encodedStateString stringByAppendingString:@"\n"]]); } - (void)testUpdateRolloutsStateToPersistenceEnsureNoHang { @@ -93,7 +101,7 @@ - (void)testUpdateRolloutsStateToPersistenceEnsureNoHang { // Clog up the queue with a long running operation. This sleep time // must be longer than the expectation timeout. - dispatch_async(FIRCLSGetLoggingQueue(), ^{ + dispatch_async(self.loggingQueue, ^{ sleep(10); }); From b1c341fd72ae979f541d905ed62b3adeece56f77 Mon Sep 17 00:00:00 2001 From: Themis wang Date: Tue, 4 Jun 2024 13:33:51 -0400 Subject: [PATCH 2/3] address comments --- .../FIRCLSRolloutsPersistenceManager.m | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m index 9f7233b0297..9dbc1133901 100644 --- a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m +++ b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m @@ -64,20 +64,22 @@ - (void)updateRolloutsStateToPersistenceWithRollouts:(NSData *_Nonnull)rollouts NSFileHandle *rolloutsFile = [NSFileHandle fileHandleForUpdatingAtPath:rolloutsPath]; - if (_rolloutsLoggingQueue) { - dispatch_async(_rolloutsLoggingQueue, ^{ - @try { - [rolloutsFile seekToEndOfFile]; - NSMutableData *rolloutsWithNewLineData = [rollouts mutableCopy]; - [rolloutsWithNewLineData appendData:[@"\n" dataUsingEncoding:NSUTF8StringEncoding]]; - [rolloutsFile writeData:rolloutsWithNewLineData]; - [rolloutsFile closeFile]; - } @catch (NSException *exception) { - FIRCLSDebugLog(@"Failed to write new rollouts. Exception name: %s - message: %s", - exception.name, exception.reason); - } - }); + if (!_rolloutsLoggingQueue) { + FIRCLSDebugLog(@"Rollouts logging queue is dealloccated"); } + + dispatch_async(_rolloutsLoggingQueue, ^{ + @try { + [rolloutsFile seekToEndOfFile]; + NSMutableData *rolloutsWithNewLineData = [rollouts mutableCopy]; + [rolloutsWithNewLineData appendData:[@"\n" dataUsingEncoding:NSUTF8StringEncoding]]; + [rolloutsFile writeData:rolloutsWithNewLineData]; + [rolloutsFile closeFile]; + } @catch (NSException *exception) { + FIRCLSDebugLog(@"Failed to write new rollouts. Exception name: %s - message: %s", + exception.name, exception.reason); + } + }); } - (void)debugLogWithMessage:(NSString *_Nonnull)message { From 286c3195b13f017a1935ba130ebc219753afff21 Mon Sep 17 00:00:00 2001 From: Themis wang Date: Tue, 4 Jun 2024 13:53:18 -0400 Subject: [PATCH 3/3] add change log --- Crashlytics/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Crashlytics/CHANGELOG.md b/Crashlytics/CHANGELOG.md index 6e6fbb5def5..5793a501571 100644 --- a/Crashlytics/CHANGELOG.md +++ b/Crashlytics/CHANGELOG.md @@ -1,3 +1,6 @@ +# # Unreleased +- [fixed] Created a new queue for rollouts persistence writes and made sure rollouts logging queue is not nil while dispatching (#12913). + # 10.27.0 - [added] Added support for catching the SIGTERM signal (#12881). - [fixed] Fixed a hang when persisting Remote Config Rollouts to disk (#12913).