-
Notifications
You must be signed in to change notification settings - Fork 467
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
fix(windows): Fix crash when running windows module #719
Conversation
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 : )
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 <4123478+tido64@users.noreply.github.com>
🎉 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
multiMerge
method 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:
Promise
class that invokes the result callback when thePromise
is resolved or rejected. To ensure that the callback is called not more than once, it usesm_isCompleted
Boolean atomic variable. To ensure that the callback is called at least once, theReject
method is called from destructor.KeyValue
andError
classes are added. These new types have customReadValue
andWriteValue
function overloads to convert to/from JSON. It allowed to stop usingJSValue
in favor of the strongly typed classes such asstd::vector
,std::string
,KeyValue
, andError
.InitializeStorage
method 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.Promise
and indicate error result by thestd::nullopt
value. Most of all functions returnstd::optional
.CHECK
andCHECK_SQL_OK
are added to simplify code by reducingif-return
statements for each function call.UniquePtrSetter
is added to allow natural use ofstd::unique_ptr
for SQL object handlers.DBTask
is not copyable, the task list changed to holdstd::unique_ptr<DBTask
instances.MultiMerge
method as other methods inside ofDBTask
. We needed to fix algorithm that matches keys between old and new values to usestd::unordered_set
instead of relying on their index which may be different and cause incorrect merges.MergeJsonObjects
code to always check value type and that key exists.Sqlite3Transaction
class code.Test Plan
The code is manually tested using the example app. The example app is generated using the following command: