-
Notifications
You must be signed in to change notification settings - Fork 478
fix(windows): Fix crash when running windows module #719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(windows): Fix crash when running windows module #719
Conversation
asklar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a few comments. Thanks for taking the time to rewrite it, looks cleaner now : )
tido64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krizzu: Do you want to take a look?
I'll leave it with you 👍 😄 |
Shorten type names suggested by @tido64 in PR review Co-authored-by: Tommy Nguyen <[email protected]>
|
🎉 This PR is included in version 1.15.14 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Lately we saw a number of Watson crash reports in Windows that show crashes due to the result callbacks called twice in case if there are errors related to the database operations. The analysis has shown that the code has no protection from the callbacks being called twice and that the
multiMergemethod is not implemented in a safe way as other methods intended to be.Details
This PR is relatively big. It is almost a full rewrite of Windows module code. These are the changes:
Promiseclass that invokes the result callback when thePromiseis resolved or rejected. To ensure that the callback is called not more than once, it usesm_isCompletedBoolean atomic variable. To ensure that the callback is called at least once, theRejectmethod is called from destructor.KeyValueandErrorclasses are added. These new types have customReadValueandWriteValuefunction overloads to convert to/from JSON. It allowed to stop usingJSValuein favor of the strongly typed classes such asstd::vector,std::string,KeyValue, andError.InitializeStoragemethod that allows to avoid using exceptions and to report errors the same way as other async task methods. This method is called on demand when a task started.Promiseand indicate error result by thestd::nulloptvalue. Most of all functions returnstd::optional.CHECKandCHECK_SQL_OKare added to simplify code by reducingif-returnstatements for each function call.UniquePtrSetteris added to allow natural use ofstd::unique_ptrfor SQL object handlers.DBTaskis not copyable, the task list changed to holdstd::unique_ptr<DBTaskinstances.MultiMergemethod as other methods inside ofDBTask. We needed to fix algorithm that matches keys between old and new values to usestd::unordered_setinstead of relying on their index which may be different and cause incorrect merges.MergeJsonObjectscode to always check value type and that key exists.Sqlite3Transactionclass code.Test Plan
The code is manually tested using the example app. The example app is generated using the following command: