Skip to content

Commit 51fea2a

Browse files
maksymmalyhinmaksymmalyhin
authored
Fix backgrounding in GDTStorage (#3627) (#3632)
* Don't end a background task twice * Ensure that setting backgroundID is atomic * Remove @synchronizeds, use a volatile variable instead * Don't run NSCoding methods on the queue All calls utilizing NSKeyedArchiver archiveXXXX should be called from on the storage queue, but it shouldn't enqueue itself. * Style * Update CHANGELOG * Change PR #s to issue #s
1 parent 7a989aa commit 51fea2a

File tree

7 files changed

+88
-63
lines changed

7 files changed

+88
-63
lines changed

‎GoogleDataTransport/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# v1.1.2
22
- Add initial support for iOS 13.
33
- Add initial support for Catalyst.
4+
- Backgrounding in GDTStorage is fixed. (#3623 and #3625)
45

56
# v1.1.1
67
- Fixes a crash in GDTUploadPackage and GDTStorage. (#3547)

‎GoogleDataTransport/GDTLibrary/GDTStorage.m‎

Lines changed: 43 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,12 @@ - (instancetype)init {
7676
- (void)storeEvent:(GDTEvent *)event {
7777
[self createEventDirectoryIfNotExists];
7878

79-
__block GDTBackgroundIdentifier bgID = GDTBackgroundIdentifierInvalid;
80-
if (_runningInBackground) {
81-
bgID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
82-
[[GDTApplication sharedApplication] endBackgroundTask:bgID];
79+
if (_backgroundID == GDTBackgroundIdentifierInvalid) {
80+
_backgroundID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
81+
if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
82+
[[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
83+
self->_backgroundID = GDTBackgroundIdentifierInvalid;
84+
}
8385
}];
8486
}
8587

@@ -110,7 +112,7 @@ - (void)storeEvent:(GDTEvent *)event {
110112
}
111113

112114
// If running in the background, save state to disk and end the associated background task.
113-
if (bgID != GDTBackgroundIdentifierInvalid) {
115+
if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
114116
if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
115117
NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
116118
requiringSecureCoding:YES
@@ -121,7 +123,8 @@ - (void)storeEvent:(GDTEvent *)event {
121123
[NSKeyedArchiver archiveRootObject:self toFile:[GDTStorage archivePath]];
122124
#endif
123125
}
124-
[[GDTApplication sharedApplication] endBackgroundTask:bgID];
126+
[[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
127+
self->_backgroundID = GDTBackgroundIdentifierInvalid;
125128
}
126129
});
127130
}
@@ -210,27 +213,34 @@ - (void)appWillForeground:(GDTApplication *)app {
210213
[NSKeyedUnarchiver unarchiveObjectWithFile:[GDTStorage archivePath]];
211214
#endif
212215
}
213-
self->_runningInBackground = NO;
214216
}
215217

216218
- (void)appWillBackground:(GDTApplication *)app {
217-
self->_runningInBackground = YES;
218-
if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
219-
NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
220-
requiringSecureCoding:YES
221-
error:nil];
222-
[data writeToFile:[GDTStorage archivePath] atomically:YES];
223-
} else {
219+
if (_backgroundID == GDTBackgroundIdentifierInvalid) {
220+
_backgroundID = [[GDTApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
221+
if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
222+
[[GDTApplication sharedApplication] endBackgroundTask:self->_backgroundID];
223+
self->_backgroundID = GDTBackgroundIdentifierInvalid;
224+
}
225+
}];
226+
}
227+
dispatch_async(_storageQueue, ^{
228+
if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
229+
NSData *data = [NSKeyedArchiver archivedDataWithRootObject:self
230+
requiringSecureCoding:YES
231+
error:nil];
232+
[data writeToFile:[GDTStorage archivePath] atomically:YES];
233+
} else {
224234
#if !defined(TARGET_OS_MACCATALYST)
225-
[NSKeyedArchiver archiveRootObject:self toFile:[GDTStorage archivePath]];
235+
[NSKeyedArchiver archiveRootObject:self toFile:[GDTStorage archivePath]];
226236
#endif
227-
}
228-
// Create an immediate background task to run until the end of the current queue of work.
229-
__block GDTBackgroundIdentifier bgID = [app beginBackgroundTaskWithExpirationHandler:^{
230-
[app endBackgroundTask:bgID];
231-
}];
237+
}
238+
});
232239
dispatch_async(_storageQueue, ^{
233-
[app endBackgroundTask:bgID];
240+
if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
241+
[app endBackgroundTask:self->_backgroundID];
242+
self->_backgroundID = GDTBackgroundIdentifierInvalid;
243+
}
234244
});
235245
}
236246

@@ -283,25 +293,19 @@ - (instancetype)initWithCoder:(NSCoder *)aDecoder {
283293

284294
- (void)encodeWithCoder:(NSCoder *)aCoder {
285295
GDTStorage *sharedInstance = [self.class sharedInstance];
286-
dispatch_queue_t storageQueue = sharedInstance.storageQueue;
287-
if (!storageQueue) {
288-
return;
296+
NSMutableOrderedSet<GDTStoredEvent *> *storedEvents = sharedInstance->_storedEvents;
297+
if (storedEvents) {
298+
[aCoder encodeObject:storedEvents forKey:kGDTStorageStoredEventsKey];
299+
}
300+
NSMutableDictionary<NSNumber *, NSMutableSet<GDTStoredEvent *> *> *targetToEventSet =
301+
sharedInstance->_targetToEventSet;
302+
if (targetToEventSet) {
303+
[aCoder encodeObject:targetToEventSet forKey:kGDTStorageTargetToEventSetKey];
304+
}
305+
GDTUploadCoordinator *uploadCoordinator = sharedInstance->_uploadCoordinator;
306+
if (uploadCoordinator) {
307+
[aCoder encodeObject:uploadCoordinator forKey:kGDTStorageUploadCoordinatorKey];
289308
}
290-
dispatch_sync(storageQueue, ^{
291-
NSMutableOrderedSet<GDTStoredEvent *> *storedEvents = sharedInstance->_storedEvents;
292-
if (storedEvents) {
293-
[aCoder encodeObject:storedEvents forKey:kGDTStorageStoredEventsKey];
294-
}
295-
NSMutableDictionary<NSNumber *, NSMutableSet<GDTStoredEvent *> *> *targetToEventSet =
296-
sharedInstance->_targetToEventSet;
297-
if (targetToEventSet) {
298-
[aCoder encodeObject:targetToEventSet forKey:kGDTStorageTargetToEventSetKey];
299-
}
300-
GDTUploadCoordinator *uploadCoordinator = sharedInstance->_uploadCoordinator;
301-
if (uploadCoordinator) {
302-
[aCoder encodeObject:uploadCoordinator forKey:kGDTStorageUploadCoordinatorKey];
303-
}
304-
});
305309
}
306310

307311
@end

‎GoogleDataTransport/GDTLibrary/Private/GDTStorage_Private.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,8 @@ NS_ASSUME_NONNULL_BEGIN
3535
/** The upload coordinator instance used by this storage instance. */
3636
@property(nonatomic) GDTUploadCoordinator *uploadCoordinator;
3737

38-
/** If YES, every call to -storeLog results in background task and serializes the singleton to disk.
39-
*/
40-
@property(nonatomic, readonly) BOOL runningInBackground;
38+
/** The background identifier, not invalid if running in the background. */
39+
@property(nonatomic) GDTBackgroundIdentifier backgroundID;
4140

4241
/** Returns the path to the keyed archive of the singleton. This is where the singleton is saved
4342
* to disk during certain app lifecycle events.

‎GoogleDataTransport/GDTLibrary/Public/GDTPlatform.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ FOUNDATION_EXPORT NSString *const kGDTApplicationWillTerminateNotification;
4242
BOOL GDTReachabilityFlagsContainWWAN(SCNetworkReachabilityFlags flags);
4343

4444
/** A typedef identify background identifiers. */
45-
typedef NSUInteger GDTBackgroundIdentifier;
45+
typedef volatile NSUInteger GDTBackgroundIdentifier;
4646

4747
/** A background task's invalid sentinel value. */
4848
FOUNDATION_EXPORT const GDTBackgroundIdentifier GDTBackgroundIdentifierInvalid;

‎GoogleDataTransport/GDTTests/Lifecycle/GDTLifecycleTest.m‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,6 @@ - (void)testBackgrounding {
107107

108108
NSNotificationCenter *notifCenter = [NSNotificationCenter defaultCenter];
109109
[notifCenter postNotificationName:kGDTApplicationDidEnterBackgroundNotification object:nil];
110-
XCTAssertTrue([GDTStorage sharedInstance].runningInBackground);
111110
XCTAssertTrue([GDTUploadCoordinator sharedInstance].runningInBackground);
112111
GDTWaitForBlock(
113112
^BOOL {
@@ -144,7 +143,6 @@ - (void)testForegrounding {
144143

145144
[[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
146145
[notifCenter postNotificationName:kGDTApplicationWillEnterForegroundNotification object:nil];
147-
XCTAssertFalse([GDTStorage sharedInstance].runningInBackground);
148146
XCTAssertFalse([GDTUploadCoordinator sharedInstance].runningInBackground);
149147
GDTWaitForBlock(
150148
^BOOL {

‎GoogleDataTransport/GDTTests/Unit/GDTStorageTest.m‎

Lines changed: 37 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@
3434
#import "GDTTests/Common/Categories/GDTRegistrar+Testing.h"
3535
#import "GDTTests/Common/Categories/GDTStorage+Testing.h"
3636

37-
static NSInteger target = 1337;
37+
static NSInteger target = kGDTTargetCCT;
3838

3939
@interface GDTStorageTest : GDTTestCase
4040

@@ -63,6 +63,8 @@ - (void)setUp {
6363

6464
- (void)tearDown {
6565
[super tearDown];
66+
dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
67+
});
6668
// Destroy these objects before the next test begins.
6769
self.testBackend = nil;
6870
self.testPrioritizer = nil;
@@ -262,16 +264,18 @@ - (void)testNSSecureCoding {
262264
event.dataObjectTransportBytes = [@"testString" dataUsingEncoding:NSUTF8StringEncoding];
263265
XCTAssertNoThrow([[GDTStorage sharedInstance] storeEvent:event]);
264266
event = nil;
265-
NSData *storageData;
266-
if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
267-
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
268-
requiringSecureCoding:YES
269-
error:nil];
270-
} else {
267+
__block NSData *storageData;
268+
dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
269+
if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
270+
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
271+
requiringSecureCoding:YES
272+
error:nil];
273+
} else {
271274
#if !defined(TARGET_OS_MACCATALYST)
272-
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
275+
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
273276
#endif
274-
}
277+
}
278+
});
275279
dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
276280
XCTAssertNotNil([[GDTStorage sharedInstance].storedEvents lastObject]);
277281
});
@@ -300,16 +304,18 @@ - (void)testNSSecureCodingWithSharedInstance {
300304
event.clockSnapshot = [GDTClock snapshot];
301305
XCTAssertNoThrow([[GDTStorage sharedInstance] storeEvent:event]);
302306
event = nil;
303-
NSData *storageData;
304-
if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
305-
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
306-
requiringSecureCoding:YES
307-
error:nil];
308-
} else {
307+
__block NSData *storageData;
308+
dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
309+
if (@available(macOS 10.13, iOS 11.0, tvOS 11.0, *)) {
310+
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]
311+
requiringSecureCoding:YES
312+
error:nil];
313+
} else {
309314
#if !defined(TARGET_OS_MACCATALYST)
310-
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
315+
storageData = [NSKeyedArchiver archivedDataWithRootObject:[GDTStorage sharedInstance]];
311316
#endif
312-
}
317+
}
318+
});
313319
dispatch_sync([GDTStorage sharedInstance].storageQueue, ^{
314320
XCTAssertNotNil([[GDTStorage sharedInstance].storedEvents lastObject]);
315321
});
@@ -354,4 +360,18 @@ - (void)testQoSTierFast {
354360
});
355361
}
356362

363+
/** Tests a deadlock condition in which a background signal is sent during -storeEvent:. */
364+
- (void)testStoreEventDeadlockFromBackgrounding {
365+
[GDTStorage sharedInstance].backgroundID = 1234;
366+
for (int i = 0; i < 10000; i++) {
367+
GDTEvent *event = [[GDTEvent alloc] initWithMappingID:@"404" target:kGDTTargetCCT];
368+
event.dataObjectTransportBytes = [@"testString" dataUsingEncoding:NSUTF8StringEncoding];
369+
event.qosTier = GDTEventQoSFast;
370+
event.clockSnapshot = [GDTClock snapshot];
371+
[[GDTStorage sharedInstance] storeEvent:event];
372+
}
373+
dispatch_sync(dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^{
374+
});
375+
}
376+
357377
@end

‎GoogleDataTransportCCTSupport/GDTCCTLibrary/GDTCCTUploader.m‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,10 @@ - (void)packageExpired:(GDTUploadPackage *)package {
192192

193193
- (void)appWillBackground:(GDTApplication *)app {
194194
_backgroundID = [app beginBackgroundTaskWithExpirationHandler:^{
195-
[app endBackgroundTask:self->_backgroundID];
195+
if (self->_backgroundID != GDTBackgroundIdentifierInvalid) {
196+
[app endBackgroundTask:self->_backgroundID];
197+
self->_backgroundID = GDTBackgroundIdentifierInvalid;
198+
}
196199
}];
197200
}
198201

0 commit comments

Comments
 (0)