Crashlytics dispatch Rollouts writes async to prevent hangs (#12977)

firebase · May 20, 2024 · dfd6149 · dfd6149
1 parent 61c10bc
commit dfd6149
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 1 deletion.
diff --git 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

diff --git a/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m b/Crashlytics/Crashlytics/Controllers/FIRCLSRolloutsPersistenceManager.m
@@ -57,12 +57,13 @@ - (void)updateRolloutsStateToPersistenceWithRollouts:(NSData *_Nonnull)rollouts
 
   NSFileHandle *rolloutsFile = [NSFileHandle fileHandleForUpdatingAtPath:rolloutsPath];
 
-  dispatch_sync(FIRCLSGetLoggingQueue(), ^{
+  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);

diff --git a/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m b/Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m
@@ -16,6 +16,7 @@
 #import <XCTest/XCTest.h>
 
 #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,43 @@ - (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];
+
+  // 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