-
Notifications
You must be signed in to change notification settings - Fork 632
[CBR-475] adds MVar for any sqlite-operation #3870
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. Serialising access to the connection on our side is much cleaner than relying on the sqlite serialised mode.
9c41f87
to
1d6cb47
Compare
1d6cb47
to
7f66ae8
Compare
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.
Fantastic, love the tests.
The `waitAny` function calls were killing threads, causing some work to not be completed.
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.
thanks @parsonsmatt for the fix. I cherry-picked only the unit-tests, without the MVars here #3872 and it fails pretty convincingly. I missed the failing stylish-Haskell frm Hydra, now fixed 🙏 . I added bindings for sqlite_threadSafe3 and tests indicate THREADSAFE
=1, which means Serialized
mode https://www.sqlite.org/threadsafe.html.
@@ -201,8 +201,7 @@ txMetaStorageSpecs = do | |||
run $ withTemporaryDb $ \hdl -> do | |||
t0 <- async $ threadWriteWithNoOp meta0 hdl | |||
t1 <- async $ threadWriteWithNoOp meta1 hdl | |||
_ <- waitAny [t0, t1] | |||
return () | |||
traverse_ wait [t0, t1] |
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.
🤦♂️ 🤦♂️
Description
Multithreading at SQlite is not the greatest. The intermediate libraries we use make things slightly worse. We already suffer from 2 known issues:
Left (Sqlite.SQLConstraintError Sqlite.Unique "tx_metas_outputs.meta_id, tx_metas_outputs.output_index")
because we get the wrong exception and the node crashes with:withTransaction
is not multithreaded. If 2 threads (which for example restore/sync at the same time) tries to use it, the node may crash. I tried this:and I get different results like:
SQLite3 returned ErrorError while attempting to perform step: not an error
SQLite3 returned ErrorError while attempting to perform step: cannot start a transaction within a transaction
so even trying to handle the IS_BUSY error, we may get the wrong error because of the previous isssue!
Given the above and despite as cruel as it is, I think we should use our own MVars and use mutually exclusive uses of the connection.
Linked issue
Type of change
How to merge
Send the message
bors r+
to merge this PR. For more information, seedocs/how-to/bors.md
.