If we wish to access database from different places on mobile devices, possibly when JS is not started we need a common function to call, that is also idempotent.
Details
Details
Build both android and iOS app and examin logs after switching between background and foreground several times to ensure that SQLiteQueryExecutor globals are initialized exactly once.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
It is great that we can have unified logic! (just a couple of inline comments)
Adding @karol-bisztyga as a reviewer
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 522 ↗ | (On Diff #9945) | initialize takes databasePath by reference, so it is a good idea that lambda used the same approach |
| 528–536 ↗ | (On Diff #9945) | It's a matter of preference, but we usually try to reduce indentation and return early, so we can consider doing the same here |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h | ||
| 7–9 ↗ | (On Diff #9945) | Do we use both mutex and thread in this header? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h | ||
|---|---|---|
| 7–9 ↗ | (On Diff #9945) | I think you might be right and <mutex> is enough to declare variable of type std::once_flag. I will check and introduce changes if it works. |
Comment Actions
Remove unused import in SQliteQueryExecutor.h, fix over-indenation and be consistent about using references.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h | ||
|---|---|---|
| 7 ↗ | (On Diff #10022) | std::once_flag is defined in <mutex> |
Comment Actions
Is it possible that this initialization might fail? We're calling it only once, so we can't retry. I don't think this is a big issue, as this failure during the initialization will make the app unusable, so a user still would need to restart it.
Adding @ashoat just to make him aware of this change.
Comment Actions
Thanks for refactoring this and reducing the code duplication!!
It doesn't look like this diff changes that, does it? Before this diff it's only called once, and after this diff it's only called once. If I'm not mistaken about that, I think this concern is separate from the diff... could be something to create a backlog task for, if you think we should eventually address it.
Requesting changes to get an answer to my question inline
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 524–525 ↗ | (On Diff #10049) | I noticed that the old code on iOS had a try-catch. What's the reason for the change? Looking at CommSecureStoreIOSWrapper.mm, it does seem like it's possible for get to throw an exception... |
Comment Actions
I think we're fine here and a new task doesn't need to be created. Previously we had the initialization code in two places so in theory it could be called twice. Additionally, if for some reason one call would fail, it was possible to call the same block again. In this new approach we're wrapping the initialization code with std::call_once so it will be called at most once. I think it's ok, as it's unlikely that after secure store initialization failure a simple retry would solve the issue. And even if it could, we would need to write some logic to do so, so at the same time we could replace std::call_once guard.
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 524–525 ↗ | (On Diff #10049) | The is a serious reason for that. Initially CommSecureStoreIOSWrapper.mm was implemented in such a way that it raised an exception when key was not present in secure store (instead of returning null). That is why we needed to try-catch to handle situation when encryption key was not generated yet and needed to be written to secure store. But recently @karol-bisztyga changed its implementation so that when key is not present in secure store simply nil is returned. It can indeed still throw an exception but not when encryption key is missing. Now if get() raises an exception it means we have a bigger problem so we do not want to ty-catch here. |
Comment Actions
Thanks for explaining! For next time, the changes to try-catch could have been separated from this diff to make it easier to review. If this diff was a simple move of the code, your reviewers wouldn't need to do a line-by-line comparison... and having the try-catch change in a separate diff would give you a venue to explain why that change was being made, so that both diffs could be accepted immediately