Skip to content
This repository was archived by the owner on Feb 19, 2020. It is now read-only.

Fix/metrics manager trackevent threadsafe #471

Merged
merged 6 commits into from
Oct 20, 2017
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
5 changes: 4 additions & 1 deletion Classes/BITChannel.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
166 changes: 116 additions & 50 deletions Classes/BITChannel.m
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#if HOCKEYSDK_FEATURE_METRICS

#import "HockeySDKPrivate.h"
#import "BITHockeyManager.h"
#import "BITChannelPrivate.h"
#import "BITHockeyHelper.h"
#import "BITTelemetryContext.h"
Expand All @@ -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";

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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:
Expand All @@ -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];
}
}

Expand All @@ -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("");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak 😟

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged the PR that fix... but I'm pretty sure we had fixed this in a prev. PR months ago. =(

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];
}
}
}
});
Expand Down Expand Up @@ -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
Expand All @@ -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);
}

Expand Down
12 changes: 6 additions & 6 deletions Classes/BITChannelPrivate.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions Classes/BITCrashManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

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