Fix/metrics manager trackevent threadsafe #471
Conversation
Classes/BITChannel.m
Outdated
| char* jsonStream = NULL; | ||
| char *newEmptyString = NULL; | ||
| do { | ||
| jsonStream = *string; |
There was a problem hiding this comment.
Swap this line with the one below it to minimize the opportunities for a race condition which forces the loop to run extra times.
Classes/BITChannel.m
Outdated
|
|
||
| // Reset both, the async-signal-safe buffer and item counter. Swaps jsonStream and BITSafeJsonEventsString. | ||
| if (OSAtomicCompareAndSwapPtr(jsonStream, newEmptyString, (void*)string)) { | ||
| self.dataItemCount = 0; |
There was a problem hiding this comment.
This is not sufficient, because self.dataItemCount is being guarded by @synchronized() in other places - Add @synchronized(self){ } around the reset of self.dataItemCount to make the setter call atomic.
There was a problem hiding this comment.
I agree with Gwynne since this method is called from the mainQueue and the dataItemsOperations queue.
Classes/BITChannel.m
Outdated
| // Enqueue item. | ||
| NSDictionary *dict = [strongSelf dictionaryForTelemetryData:item]; | ||
| [strongSelf appendDictionaryToEventBuffer:dict]; | ||
| if (strongSelf.dataItemCount >= strongSelf.maxBatchSize) { |
There was a problem hiding this comment.
You need an @synchronized(self) here as well to guard the access to the data item count.
Classes/BITChannel.m
Outdated
| 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); |
There was a problem hiding this comment.
This is not safe - you're still accessing a previousBuffer which may have been changed out by another thread. You have to compare and swap it out first before using its value.
Classes/BITChannel.m
Outdated
| return; | ||
| } | ||
|
|
||
| NSData *bundle = [NSData dataWithBytes:BITSafeJsonEventsString length:strlen(BITSafeJsonEventsString)]; | ||
| NSData *bundle = [NSData dataWithBytes:jsonStream length:strlen(jsonStream)]; | ||
| [self.persistence persistBundle:bundle]; |
There was a problem hiding this comment.
Is persistence thread safe too? Meaning can save 2 bundles at the same time without conflicts?
Classes/BITChannel.m
Outdated
|
|
||
| // Reset both, the async-signal-safe buffer and item counter. Swaps jsonStream and BITSafeJsonEventsString. | ||
| if (OSAtomicCompareAndSwapPtr(jsonStream, newEmptyString, (void*)string)) { | ||
| self.dataItemCount = 0; |
There was a problem hiding this comment.
I agree with Gwynne since this method is called from the mainQueue and the dataItemsOperations queue.
Classes/BITChannel.m
Outdated
| [self invalidateTimer]; | ||
| if (!BITSafeJsonEventsString || strlen(BITSafeJsonEventsString) == 0) { | ||
| char* jsonStream = NULL; | ||
| char *newEmptyString = NULL; |
There was a problem hiding this comment.
🔧 One of the asterisks here is off 🙂
There was a problem hiding this comment.
thx @lumaxis! I fixed that one.
|
I discussed this PR with @gwynne to a great length. While my approach above doesn't 100% guarantee safety, it's a step in the right direction. Please let me know if you have objections, otherwise I'll merge the PR. |
…ToEventBuffer itself is not threadsafe, it needs to be called inside a @synchronized block which it usually is.
|
I talked to Gwynne about this to a great deal and bottom line is that this PR is as best as we can do without refactoring the whole buffer logic into something that uses a proper, threadsafe linked list (similar to what we use for Mobile Center). |
ElektrojungeAtWork
deleted the
fix/MetricsManager-trackevent-threadsafe
branch
| char *previousBuffer = NULL; | ||
| char *newEmptyString = NULL; | ||
| do { | ||
| newEmptyString = strdup(""); |
There was a problem hiding this comment.
Merged the PR that fix... but I'm pretty sure we had fixed this in a prev. PR months ago. =(
Hopefully a more thorough approach to making BITChannel thread-safe.
I'm combining mutexes and atomic compare and swap based on https://github.com/bitstadium/HockeySDK-iOS/pulls and bitstadium/HockeySDK-Mac#106.
From testing, this works great. I'm happy for as many pairs of eyes as possible.