From 5d506be10c84e00d21bb2b67b0f355131eeffd09 Mon Sep 17 00:00:00 2001 From: samedson Date: Thu, 16 May 2024 10:43:30 -0400 Subject: [PATCH 1/6] Crashlytics dispatch Rollouts writes async to prevent hangs --- .../FIRCLSRolloutsPersistenceManager.m | 2 +- .../FIRCLSRolloutsPersistenceManagerTests.m | 43 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m index defd9d68a52..dc377f2c8c2 100644 --- a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m +++ b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m @@ -57,7 +57,7 @@ - (void)updateRolloutsStateToPersistenceWithRollouts:(NSData *_Nonnull)rollouts NSFileHandle *rolloutsFile = [NSFileHandle fileHandleForUpdatingAtPath:rolloutsPath]; - dispatch_sync(FIRCLSGetLoggingQueue(), ^{ + dispatch_async(FIRCLSGetLoggingQueue(), ^{ @try { [rolloutsFile seekToEndOfFile]; NSMutableData *rolloutsWithNewLineData = [rollouts mutableCopy]; diff --git a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m index aec030d7538..d8093e5acea 100644 --- a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m @@ -16,6 +16,7 @@ #import #import "Crashlytics/Crashlytics/Components/FIRCLSContext.h" +#include "Crashlytics/Crashlytics/Components/FIRCLSGlobals.h" #import "Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.h" #import "Crashlytics/Crashlytics/Models/FIRCLSInternalReport.h" #import "Crashlytics/UnitTests/Mocks/FIRCLSTempMockFileManager.h" @@ -51,6 +52,9 @@ - (void)tearDown { } - (void)testUpdateRolloutsStateToPersistenceWithRollouts { + XCTestExpectation *expectation = [[XCTestExpectation alloc] + initWithDescription:@"Expect updating rollouts to finish writing."]; + NSString *encodedStateString = @"{rollouts:[{\"parameter_key\":\"6d795f66656174757265\",\"parameter_value\":" @"\"e8bf99e698af7468656d6973e79a84e6b58be8af95e695b0e68daeefbc8ce8be93e585a5e4b8ade69687\"," @@ -64,7 +68,46 @@ - (void)testUpdateRolloutsStateToPersistenceWithRollouts { [self.rolloutsPersistenceManager updateRolloutsStateToPersistenceWithRollouts:data reportID:reportId]; + + // Wait for the logging queue to finish. + dispatch_async(FIRCLSGetLoggingQueue(), ^{ + [expectation fulfill]; + }); + + [self waitForExpectations:@[expectation] timeout:3]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath:rolloutsFilePath]); } +- (void)testUpdateRolloutsStateToPersistenceEnsureNoHang { + dispatch_queue_t testQueue = dispatch_queue_create("TestQueue", DISPATCH_QUEUE_SERIAL); + XCTestExpectation *expectation = [[XCTestExpectation alloc] + initWithDescription:@"Expect updating rollouts to return."]; + NSString *encodedStateString = + @"{rollouts:[{\"parameter_key\":\"6d795f66656174757265\",\"parameter_value\":" + @"\"e8bf99e698af7468656d6973e79a84e6b58be8af95e695b0e68daeefbc8ce8be93e585a5e4b8ade69687\"," + @"\"rollout_id\":\"726f6c6c6f75745f31\",\"template_version\":1,\"variant_id\":" + @"\"636f6e74726f6c\"}]}"; + + NSData *data = [encodedStateString dataUsingEncoding:NSUTF8StringEncoding]; + NSString *rolloutsFilePath = + [[[self.fileManager activePath] stringByAppendingPathComponent:reportId] + stringByAppendingPathComponent:FIRCLSReportRolloutsFile]; + + // Clog up the queue with a long running operation. This sleep time + // must be longer than the expectation timeout. + dispatch_async(FIRCLSGetLoggingQueue(), ^{ + sleep(10); + }); + + dispatch_async(testQueue, ^{ + // Ensure that calling this returns quickly so we don't hang + [self.rolloutsPersistenceManager updateRolloutsStateToPersistenceWithRollouts:data + reportID:reportId]; + [expectation fulfill]; + }); + + [self waitForExpectations:@[expectation] timeout:3]; +} + @end From f77a3515aefb9b030f144c8341ac235fd77a18e3 Mon Sep 17 00:00:00 2001 From: samedson Date: Thu, 16 May 2024 10:47:08 -0400 Subject: [PATCH 2/6] Changelog --- Crashlytics/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Crashlytics/CHANGELOG.md b/Crashlytics/CHANGELOG.md index f65bacfc228..907e2cfe92c 100644 --- a/Crashlytics/CHANGELOG.md +++ b/Crashlytics/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased - [added] Added support for catching the SIGTERM signal (#12881). +- [fixed] Fixed a hang when persisting Remote Config Rollouts to disk (#12913). # 10.25.0 - [changed] Removed usages of user defaults API from internal Firebase Sessions From 642e13b7e7fb190a09707373dd664dc36be773be Mon Sep 17 00:00:00 2001 From: samedson Date: Thu, 16 May 2024 10:55:10 -0400 Subject: [PATCH 3/6] Style --- .../UnitTests/FIRCLSRolloutsPersistenceManagerTests.m | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m index d8093e5acea..ded6428e153 100644 --- a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m @@ -53,7 +53,7 @@ - (void)tearDown { - (void)testUpdateRolloutsStateToPersistenceWithRollouts { XCTestExpectation *expectation = [[XCTestExpectation alloc] - initWithDescription:@"Expect updating rollouts to finish writing."]; + initWithDescription:@"Expect updating rollouts to finish writing."]; NSString *encodedStateString = @"{rollouts:[{\"parameter_key\":\"6d795f66656174757265\",\"parameter_value\":" @@ -74,15 +74,15 @@ - (void)testUpdateRolloutsStateToPersistenceWithRollouts { [expectation fulfill]; }); - [self waitForExpectations:@[expectation] timeout:3]; + [self waitForExpectations:@[ expectation ] timeout:3]; XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath:rolloutsFilePath]); } - (void)testUpdateRolloutsStateToPersistenceEnsureNoHang { dispatch_queue_t testQueue = dispatch_queue_create("TestQueue", DISPATCH_QUEUE_SERIAL); - XCTestExpectation *expectation = [[XCTestExpectation alloc] - initWithDescription:@"Expect updating rollouts to return."]; + XCTestExpectation *expectation = + [[XCTestExpectation alloc] initWithDescription:@"Expect updating rollouts to return."]; NSString *encodedStateString = @"{rollouts:[{\"parameter_key\":\"6d795f66656174757265\",\"parameter_value\":" @"\"e8bf99e698af7468656d6973e79a84e6b58be8af95e695b0e68daeefbc8ce8be93e585a5e4b8ade69687\"," @@ -107,7 +107,7 @@ - (void)testUpdateRolloutsStateToPersistenceEnsureNoHang { [expectation fulfill]; }); - [self waitForExpectations:@[expectation] timeout:3]; + [self waitForExpectations:@[ expectation ] timeout:3]; } @end From 88435e1bde854bf02494e0aa8220e7f8e2d6c610 Mon Sep 17 00:00:00 2001 From: samedson Date: Thu, 16 May 2024 14:48:26 -0400 Subject: [PATCH 4/6] Persistence --- Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m | 3 --- 1 file changed, 3 deletions(-) diff --git a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m index ded6428e153..4caceb242b1 100644 --- a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m @@ -90,9 +90,6 @@ - (void)testUpdateRolloutsStateToPersistenceEnsureNoHang { @"\"636f6e74726f6c\"}]}"; NSData *data = [encodedStateString dataUsingEncoding:NSUTF8StringEncoding]; - NSString *rolloutsFilePath = - [[[self.fileManager activePath] stringByAppendingPathComponent:reportId] - stringByAppendingPathComponent:FIRCLSReportRolloutsFile]; // Clog up the queue with a long running operation. This sleep time // must be longer than the expectation timeout. From 4a526194657e6752a9cfa259244d26532128fec5 Mon Sep 17 00:00:00 2001 From: samedson Date: Fri, 17 May 2024 12:34:41 -0400 Subject: [PATCH 5/6] Remove expectations --- .../UnitTests/FIRCLSRolloutsPersistenceManagerTests.m | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m index 4caceb242b1..5145b8adf91 100644 --- a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m @@ -52,9 +52,6 @@ - (void)tearDown { } - (void)testUpdateRolloutsStateToPersistenceWithRollouts { - XCTestExpectation *expectation = [[XCTestExpectation alloc] - initWithDescription:@"Expect updating rollouts to finish writing."]; - NSString *encodedStateString = @"{rollouts:[{\"parameter_key\":\"6d795f66656174757265\",\"parameter_value\":" @"\"e8bf99e698af7468656d6973e79a84e6b58be8af95e695b0e68daeefbc8ce8be93e585a5e4b8ade69687\"," @@ -69,13 +66,6 @@ - (void)testUpdateRolloutsStateToPersistenceWithRollouts { [self.rolloutsPersistenceManager updateRolloutsStateToPersistenceWithRollouts:data reportID:reportId]; - // Wait for the logging queue to finish. - dispatch_async(FIRCLSGetLoggingQueue(), ^{ - [expectation fulfill]; - }); - - [self waitForExpectations:@[ expectation ] timeout:3]; - XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath:rolloutsFilePath]); } From 3a5e64d8b138e82208506a08ec1ee5449606b484 Mon Sep 17 00:00:00 2001 From: samedson Date: Fri, 17 May 2024 13:28:51 -0400 Subject: [PATCH 6/6] Close the file --- .../Controllers/FIRCLSRolloutsPersistenceManager.m | 1 + .../UnitTests/FIRCLSRolloutsPersistenceManagerTests.m | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m index dc377f2c8c2..57993a965f3 100644 --- a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m +++ b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m @@ -63,6 +63,7 @@ - (void)updateRolloutsStateToPersistenceWithRollouts:(NSData *_Nonnull)rollouts 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); diff --git a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m index 5145b8adf91..4caceb242b1 100644 --- a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m +++ b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m @@ -52,6 +52,9 @@ - (void)tearDown { } - (void)testUpdateRolloutsStateToPersistenceWithRollouts { + XCTestExpectation *expectation = [[XCTestExpectation alloc] + initWithDescription:@"Expect updating rollouts to finish writing."]; + NSString *encodedStateString = @"{rollouts:[{\"parameter_key\":\"6d795f66656174757265\",\"parameter_value\":" @"\"e8bf99e698af7468656d6973e79a84e6b58be8af95e695b0e68daeefbc8ce8be93e585a5e4b8ade69687\"," @@ -66,6 +69,13 @@ - (void)testUpdateRolloutsStateToPersistenceWithRollouts { [self.rolloutsPersistenceManager updateRolloutsStateToPersistenceWithRollouts:data reportID:reportId]; + // Wait for the logging queue to finish. + dispatch_async(FIRCLSGetLoggingQueue(), ^{ + [expectation fulfill]; + }); + + [self waitForExpectations:@[ expectation ] timeout:3]; + XCTAssertTrue([[NSFileManager defaultManager] fileExistsAtPath:rolloutsFilePath]); }