Merge pull request #471 from bitstadium/fix/MetricsManager-trackevent-threadsafe

Fix/metrics manager trackevent threadsafe

Author: Benjamin Scholtysik (Reimold)

Classes/BITChannel.h

@@ -11,7 +11,10 @@
 #import "HockeySDKNullability.h"
 NS_ASSUME_NONNULL_BEGIN
 
-FOUNDATION_EXPORT char *BITSafeJsonEventsString;
+/**
+ * Buffer of telemtry events, will be written to disk. Make sure the buffer is used in a threadsafe way.
+ */
+FOUNDATION_EXPORT char *_Nullable BITTelemetryEventBuffer;
 
 /**
  *  Items get queued before they are persisted and sent out as a batch. This class managed the queue, and forwards the batch

Classes/BITChannel.m

@@ -3,6 +3,7 @@
 #if HOCKEYSDK_FEATURE_METRICS
 
 #import "HockeySDKPrivate.h"
+#import "BITHockeyManager.h"
 #import "BITChannelPrivate.h"
 #import "BITHockeyHelper.h"
 #import "BITTelemetryContext.h"
@@ -12,9 +13,11 @@
 #import "BITData.h"
 #import "BITDevice.h"
 #import "BITPersistencePrivate.h"
+#import <libkern/OSAtomic.h>
+
 
 static char *const BITDataItemsOperationsQueue = "net.hockeyapp.senderQueue";
-char *BITSafeJsonEventsString;
+char *BITTelemetryEventBuffer;
 
 NSString *const BITChannelBlockedNotification = @"BITChannelBlockedNotification";
 
@@ -42,7 +45,7 @@ @implementation BITChannel
 
 - (instancetype)init {
   if ((self = [super init])) {
-    bit_resetSafeJsonStream(&BITSafeJsonEventsString);
+    bit_resetEventBuffer(&BITTelemetryEventBuffer);
     _dataItemCount = 0;
     if (bit_isDebuggerAttached()) {
       _maxBatchSize = BITDebugMaxBatchSize;
@@ -80,7 +83,6 @@ - (void) registerObservers {
     void (^notificationBlock)(NSNotification *note) = ^(NSNotification __unused *note) {
       typeof(self) strongSelf = weakSelf;
       if ([strongSelf timerIsRunning]) {
-        [strongSelf persistDataItemQueue];
 
         /**
          * From the documentation for applicationDidEnterBackground:
@@ -93,12 +95,15 @@ - (void) registerObservers {
           [sharedApplication endBackgroundTask:_backgroundTask];
           _backgroundTask = UIBackgroundTaskInvalid;
         }];
+        
+        // Do background work that will be done in it's own async queue.
+        [strongSelf persistDataItemQueue:&BITTelemetryEventBuffer];
       }
     };
     self.appDidEnterBackgroundObserver = [[NSNotificationCenter defaultCenter] addObserverForName:UIApplicationDidEnterBackgroundNotification
-                                                                                       object:nil
-                                                                                        queue:NSOperationQueue.mainQueue
-                                                                                   usingBlock:notificationBlock];
+                                                                                           object:nil
+                                                                                            queue:NSOperationQueue.mainQueue
+                                                                                       usingBlock:notificationBlock];
   }
 }
 
@@ -123,59 +128,89 @@ - (BOOL)isQueueBusy {
   return self.channelBlocked;
 }
 
-- (void)persistDataItemQueue {
+- (void)persistDataItemQueue:(char **)eventBuffer {
   [self invalidateTimer];
-  if (!BITSafeJsonEventsString || strlen(BITSafeJsonEventsString) == 0) {
+  
+  // Make sure string (which points to BITTelemetryEventBuffer) is not changed.
+  char *previousBuffer = NULL;
+  char *newEmptyString = NULL;
+  do {
+    newEmptyString = strdup("");
+    previousBuffer = *eventBuffer;
+    
+    // This swaps pointers and makes sure eventBuffer now has the balue of newEmptyString.
+    if (OSAtomicCompareAndSwapPtr(previousBuffer, newEmptyString, (void*)eventBuffer)) {
+      @synchronized(self) {
+        self.dataItemCount = 0;
+      }
+      break;
+    }
+  } while(true);
+  
+  // Nothing to persist, freeing memory and existing.
+  if (!previousBuffer || strlen(previousBuffer) == 0) {
+    free(previousBuffer);
     return;
   }
 
-  NSData *bundle = [NSData dataWithBytes:BITSafeJsonEventsString length:strlen(BITSafeJsonEventsString)];
+  // Persist the data
+  NSData *bundle = [NSData dataWithBytes:previousBuffer length:strlen(previousBuffer)];
   [self.persistence persistBundle:bundle];
+  free(previousBuffer);
 
   // Reset both, the async-signal-safe and item counter.
   [self resetQueue];
 }
 
+// Resets the event buffer and count of events in the queue.
 - (void)resetQueue {
-  bit_resetSafeJsonStream(&BITSafeJsonEventsString);
-  self.dataItemCount = 0;
+  @synchronized (self) {
+    bit_resetEventBuffer(&BITTelemetryEventBuffer);
+    self.dataItemCount = 0;
+  }
 }
 
 #pragma mark - Adding to queue
 
 - (void)enqueueTelemetryItem:(BITTelemetryData *)item {
 
   if (!item) {
-    // Case 1: Item is nil: Do not enqueue item and abort operation
+    
+    // Item is nil: Do not enqueue item and abort operation.
     BITHockeyLogWarning(@"WARNING: TelemetryItem was nil.");
     return;
   }
 
+  // First assigning self to weakSelf and then assigning this to strongSelf in the block is not very intuitive, this
+  // blog post explains it very well: https://dhoerl.wordpress.com/2013/04/23/i-finally-figured-out-weakself-and-strongself/
   __weak typeof(self) weakSelf = self;
   dispatch_async(self.dataItemsOperations, ^{
-    typeof(self) strongSelf = weakSelf;
 
+    typeof(self) strongSelf = weakSelf;
     if (strongSelf.isQueueBusy) {
-      // Case 2: Channel is in blocked state: Trigger sender, start timer to check after again after a while and abort operation.
+      
+      // Case 1: Channel is in blocked state: Trigger sender, start timer to check after again after a while and abort operation.
       BITHockeyLogDebug(@"INFO: The channel is saturated. %@ was dropped.", item.debugDescription);
       if (![strongSelf timerIsRunning]) {
         [strongSelf startTimer];
       }
       return;
     }
 
-    // Enqueue item
-    NSDictionary *dict = [self dictionaryForTelemetryData:item];
-    [strongSelf appendDictionaryToJsonStream:dict];
-    
-    if (strongSelf.dataItemCount >= self.maxBatchSize) {
-      // Case 3: Max batch count has been reached, so write queue to disk and delete all items.
-      [strongSelf persistDataItemQueue];
-      
-    } else if (strongSelf.dataItemCount == 1) {
-      // Case 4: It is the first item, let's start the timer.
-      if (![strongSelf timerIsRunning]) {
-        [strongSelf startTimer];
+    // Enqueue item.
+    @synchronized(self) {
+      NSDictionary *dict = [strongSelf dictionaryForTelemetryData:item];
+      [strongSelf appendDictionaryToEventBuffer:dict];
+      if (strongSelf.dataItemCount >= strongSelf.maxBatchSize) {
+        
+        // Case 2: Max batch count has been reached, so write queue to disk and delete all items.
+        [strongSelf persistDataItemQueue:&BITTelemetryEventBuffer];
+      } else if (strongSelf.dataItemCount > 0) {
+        
+        // Case 3: It is the first item, let's start the timer.
+        if (![strongSelf timerIsRunning]) {
+          [strongSelf startTimer];
+        }
       }
     }
   });
@@ -223,39 +258,72 @@ - (NSString *)serializeDictionaryToJSONString:(NSDictionary *)dictionary {
 
 #pragma mark JSON Stream
 
-- (void)appendDictionaryToJsonStream:(NSDictionary *)dictionary {
+- (void)appendDictionaryToEventBuffer:(NSDictionary *)dictionary {
   if (dictionary) {
     NSString *string = [self serializeDictionaryToJSONString:dictionary];
 
     // Since we can't persist every event right away, we write it to a simple C string.
     // This can then be written to disk by a signal handler in case of a crash.
-    bit_appendStringToSafeJsonStream(string, &(BITSafeJsonEventsString));
-    self.dataItemCount += 1;
+    @synchronized (self) {
+      bit_appendStringToEventBuffer(string, &BITTelemetryEventBuffer);
+      self.dataItemCount += 1;
+    }
   }
 }
 
-void bit_appendStringToSafeJsonStream(NSString *string, char **jsonString) {
-  if (jsonString == NULL) { return; }
+void bit_appendStringToEventBuffer(NSString *string, char **eventBuffer) {
+  if (eventBuffer == NULL) {
+    return;
+  }
 
-  if (!string) { return; }
+  if (!string) {
+    return;
+  }
 
-  if (*jsonString == NULL || strlen(*jsonString) == 0) {
-    bit_resetSafeJsonStream(jsonString);
+  if (*eventBuffer == NULL || strlen(*eventBuffer) == 0) {
+    bit_resetEventBuffer(eventBuffer);
   }
 
-  if (string.length == 0) { return; }
+  if (string.length == 0) {
+    return;
+  }
 
-  char *new_string = NULL;
-  // Concatenate old string with new JSON string and add a comma.
-  asprintf(&new_string, "%s%.*s\n", *jsonString, (int)MIN(string.length, (NSUInteger)INT_MAX), string.UTF8String);
-  free(*jsonString);
-  *jsonString = new_string;
+  do {
+    char *newBuffer = NULL;
+    char *previousBuffer = *eventBuffer;
+    
+    // Concatenate old string with new JSON string and add a comma.
+    asprintf(&newBuffer, "%s%.*s\n", previousBuffer, (int)MIN(string.length, (NSUInteger)INT_MAX), string.UTF8String);
+    
+    // Compare newBuffer and previousBuffer. If they point to the same address, we are safe to use them.
+    if (OSAtomicCompareAndSwapPtr(previousBuffer, newBuffer, (void*)eventBuffer)) {
+ 
+      // Free the intermediate pointer.
+      free(previousBuffer);
+      return;
+    } else {
+      
+      // newBuffer has been changed by another thread.
+      free(newBuffer);
+    }
+  } while (true);
 }
 
-void bit_resetSafeJsonStream(char **string) {
-  if (!string) { return; }
-  free(*string);
-  *string = strdup("");
+void bit_resetEventBuffer(char **eventBuffer) {
+  if (!eventBuffer) { return; }
+  
+  char *newEmptyString = NULL;
+  char *prevString = NULL;
+  do {
+    prevString = *eventBuffer;
+    newEmptyString = strdup("");
+    
+    // Compare pointers to strings to make sure we are still threadsafe!
+    if (OSAtomicCompareAndSwapPtr(prevString, newEmptyString, (void*)eventBuffer)) {
+      free(prevString);
+      return;
+    }
+  } while(true);
 }
 
 #pragma mark - Batching
@@ -279,28 +347,26 @@ -(BOOL)timerIsRunning {
 }
 
 - (void)startTimer {
-  // Reset timer, if it is already running
+  
+  // Reset timer, if it is already running.
   if ([self timerIsRunning]) {
     [self invalidateTimer];
   }
 
   self.timerSource = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, self.dataItemsOperations);
   dispatch_source_set_timer((dispatch_source_t)self.timerSource, dispatch_walltime(NULL, NSEC_PER_SEC * self.batchInterval), 1ull * NSEC_PER_SEC, 1ull * NSEC_PER_SEC);
-  
   __weak typeof(self) weakSelf = self;
   dispatch_source_set_event_handler((dispatch_source_t)self.timerSource, ^{
     typeof(self) strongSelf = weakSelf;
-    
-    if(strongSelf) {
+    if (strongSelf) {
       if (strongSelf.dataItemCount > 0) {
-        [strongSelf persistDataItemQueue];
+        [strongSelf persistDataItemQueue:&BITTelemetryEventBuffer];
       } else {
         strongSelf.channelBlocked = NO;
       }
       [strongSelf invalidateTimer];
     }
   });
-  
   dispatch_resume((dispatch_source_t)self.timerSource);
 }
 

Classes/BITChannelPrivate.h

@@ -70,28 +70,28 @@ FOUNDATION_EXPORT NSString *const BITChannelBlockedNotification;
 /**
  *  Manually trigger the BITChannel to persist all items currently in its data item queue.
  */
-- (void)persistDataItemQueue;
+- (void)persistDataItemQueue:(char *_Nullable*_Nullable)eventBuffer;
 
 /**
  *  Adds the specified dictionary to the JSON Stream string.
  *
  *  @param dictionary the dictionary object which is to be added to the JSON Stream queue string.
  */
-- (void)appendDictionaryToJsonStream:(NSDictionary *)dictionary;
+- (void)appendDictionaryToEventBuffer:(NSDictionary *)dictionary;
 
 /**
  *  A C function that serializes a given dictionary to JSON and appends it to a char string
  *
  *  @param string The C string which the dictionary's JSON representation will be appended to.
  */
-void bit_appendStringToSafeJsonStream(NSString *string, char *__nonnull*__nonnull jsonStream);
+void bit_appendStringToEventBuffer(NSString *string, char *__nonnull*__nonnull eventBuffer);
 
 /**
- *  Reset BITSafeJsonEventsString so we can start appending JSON dictionaries.
+ *  Reset the event buffer so we can start appending JSON dictionaries.
  *
- *  @param jsonStream The string that will be reset.
+ *  @param eventBuffer The string that will be reset.
  */
-void bit_resetSafeJsonStream(char *__nonnull*__nonnull jsonStream);
+void bit_resetEventBuffer(char *__nonnull*__nonnull eventBuffer);
 
 /**
  *  A method which indicates whether the telemetry pipeline is busy and no new data should be enqueued.

Classes/BITCrashManager.m

@@ -107,7 +107,7 @@
 static void bit_save_events_callback(siginfo_t __unused *info, ucontext_t __unused *uap, void __unused *context) {
 
   // Do not flush metrics queue if queue is empty (metrics module disabled) to not freeze the app
-  if (!BITSafeJsonEventsString) {
+  if (!BITTelemetryEventBuffer) {
     return;
   }
 
@@ -117,10 +117,10 @@ static void bit_save_events_callback(siginfo_t __unused *info, ucontext_t __unus
     return;
   }
 
-  size_t len = strlen(BITSafeJsonEventsString);
+  size_t len = strlen(BITTelemetryEventBuffer);
   if (len > 0) {
     // Simply write the whole string to disk
-    write(fd, BITSafeJsonEventsString, len);
+    write(fd, BITTelemetryEventBuffer, len);
   }
   close(fd);
 }