Multi Triggers -> Develop #395
Conversation
- TODO: Call IsEventFirstTime from the new db/localDataStore class
- Renamed one letter object names to fit the context - TODO: Call appropriate persist method from the new db/localDataStore class
- Added new class CTEventDatabase to store user events in database. - Added create table, insert, update and check event methods.
…ofile-events [MC-2088] Evaluate first time profile events
…ents [MC-2086] Evaluate first time events
- synchronised userEventLogs set access
[MC - 2090] In-memory storage events
[MC-2106] Add event storage limits
[MC-2082] Implement new event storage
[MC-2430] Normalize event names
…e to the database. updated tests. added normalised names to queries.
Complete Todos and connect database code
There was a problem hiding this comment.
In - (void)processEvent:(NSDictionary *)event withType:(CleverTapEventType)eventType the event is written to the local storage and further down in the same method the evaluation manager is called to evaluate the in-apps for the event.
The local data store calls persistEvent which calls self.dbHelper upsertEvent in a background queue with dispatch_async. The database then runs the query in the database queue.
The evaluation is done on the serial queue on which the events are processed and queued.
The current flow is persistEvent, upsertEvent, evaluateOnEvent, eventExists, update success, upsert success. However, this does not seem guaranteed since code runs on different queues. This opens for potential race conditions. Please check this.
Please check the comments left. I have not finished reviewing all files but I believe the feedback provided is enough for discussion. I will leave additional comments if I have more suggestions.
| dispatch_queue_t _databaseQueue; | ||
| } | ||
|
|
||
| + (instancetype)sharedInstanceWithConfig:(CleverTapInstanceConfig *)config { |
There was a problem hiding this comment.
The CTEventDatabase shared instance with config is bit misleading that it can return a different instance based on the config and use a shared instance with same configs. Do we want separate databases per account id?
The CTEventDatabase is only used in the local store, does it need to be a singleton?
There was a problem hiding this comment.
Removed config part and still used singleton as local store init can be called from other CT instance also. We are not using separate database per account id here. Any suggestion if we want to use separate database per account id?
Completed Todos. Updated Tests, Removed sleep.
… avoid deadlock - Fixes race condition in persist event due to different serial queue. - Updated unit tests.
@nzagorchev I have updated code to avoid race condition, used same dispatch manager serial queue in db operations also. Kindly review new changes. |
- Added proper return for error and records not found. - Removed get first and last time public API as per TAN. - Minor code improvements.
| } | ||
| } | ||
| NSInteger count = [self.dbHelper getEventCount:normalizedName deviceID:self.deviceInfo.deviceId]; | ||
| if (count > 1) { |
There was a problem hiding this comment.
Shouldn't this check be count > 0?
There was a problem hiding this comment.
This is updated as per TAN, as the event is stored in the LocalDataStore before the evaluation. This means a record for the event will always exist in the data store. If the event count is 1, it is a first time event. Refer: https://wizrocket.atlassian.net/wiki/spaces/EN/pages/4464115774/TAN+SDK+In-app+Multi-triggers#LocalDataStore
Also the same changes are in Android PR: https://github.com/CleverTap/clevertap-android-sdk/pull/683/files#diff-3e307aa90ae0394b024172f2b91b3c392c33c49380e4895becadaab6633f4e86R283
There was a problem hiding this comment.
yes, this is for the boolean check count == 1 but if it is recorded in the database, we can add it to the set.
There was a problem hiding this comment.
Yes got it, Should I update it like:
NSInteger count = [self.dbHelper getEventCount:normalizedName deviceID:self.deviceInfo.deviceId];
if (count > 0) {
@synchronized (self.userEventLogs) {
[self.userEventLogs addObject:normalizedName];
}
}
return count == 1;
```
There was a problem hiding this comment.
There are potential deadlocks where the dispatch_semaphore_wait is used in this class' methods.
The implementation can cause a deadlock if called from the same serial queue managed by the DispatchQueueManager. This happens because:
- The method uses a semaphore to wait for a task to complete.
- If the method is called while already running on the serial queue, the
dispatch_asyncin runSerialAsync:will execute the task synchronously. The semaphore wait will block the queue, preventing the task from completing and thus causing a deadlock.
There are two options to solve this:
- Rewrite the code to add checks where the method is run from:
void (^taskBlock)(void) = ^{
sqlite3_stmt *statement;
...
sqlite3_finalize(statement);
};
if ([self.dispatchQueueManager inSerialQueue]) {
// If already on the serial queue, execute directly without semaphore
taskBlock();
} else {
// Otherwise, use semaphore for synchronous execution
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
[self.dispatchQueueManager runSerialAsync:^{
taskBlock();
dispatch_semaphore_signal(semaphore);
}];
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
}
return result;
- Rewrite the code to use completion blocks.
[self.dispatchQueueManager runSerialAsync:^{
...
if (completion) {
// Dispatch callback on main queue
dispatch_async(dispatch_get_main_queue(), ^{
completion(result);
});
}
}];
There was a problem hiding this comment.
Updated code to use option 1 by adding semaphore only when it is not in serial queue, kindly review the changes for this.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
… running in dispatch queue manager serial queue.
No description provided.