Skip to content
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

Make sure rollouts logging queue is not nil #13058

Merged
merged 3 commits into from
Jun 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions Crashlytics/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@

@interface FIRCLSRolloutsPersistenceManager : NSObject <FIRCLSPersistenceLog>

- (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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.

#import <Foundation/Foundation.h>
#include "Crashlytics/Crashlytics/Components/FIRCLSGlobals.h"
#include "Crashlytics/Crashlytics/Components/FIRCLSUserLogging.h"
#import "Crashlytics/Crashlytics/Helpers/FIRCLSLogger.h"
#import "Crashlytics/Crashlytics/Models/FIRCLSFileManager.h"
Expand All @@ -32,15 +31,23 @@

@interface FIRCLSRolloutsPersistenceManager : NSObject <FIRCLSPersistenceLog>
@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;
}

Expand All @@ -57,7 +64,11 @@ - (void)updateRolloutsStateToPersistenceWithRollouts:(NSData *_Nonnull)rollouts

NSFileHandle *rolloutsFile = [NSFileHandle fileHandleForUpdatingAtPath:rolloutsPath];

dispatch_async(FIRCLSGetLoggingQueue(), ^{
if (!_rolloutsLoggingQueue) {
FIRCLSDebugLog(@"Rollouts logging queue is dealloccated");
}
ncooke3 marked this conversation as resolved.
Show resolved Hide resolved

dispatch_async(_rolloutsLoggingQueue, ^{
@try {
[rolloutsFile seekToEndOfFile];
NSMutableData *rolloutsWithNewLineData = [rollouts mutableCopy];
Expand Down
6 changes: 5 additions & 1 deletion Crashlytics/Crashlytics/FIRCrashlytics.m
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
18 changes: 13 additions & 5 deletions Crashlytics/UnitTests/FIRCLSRolloutsPersistenceManagerTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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);
});

Expand Down
Loading