This differential implements simplistic GlobalDBSingleton API for safe access to database from diffferent parts of the code. Its simplicity stems from the fact that Android and iOS will use different synchronization that will be implemented in platform-specific code.
Details
Details
- Reviewers
tomek atul • jon - Commits
- rCOMMd64d2f132e42: Define global singleton db C++ API.
Build the app.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.cpp | ||
|---|---|---|
| 12–16 ↗ | (On Diff #17098) | Not sure how others feel, but I think I would rather have if else than an early return |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.cpp | ||
|---|---|---|
| 12–16 ↗ | (On Diff #17098) | I recall being advised several times by other reviewers in this project to keep indenation at an absolute minumum and if + early return produces less indenation that if else. |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.cpp | ||
|---|---|---|
| 3 ↗ | (On Diff #17126) | Are we using this import? |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
| 5 ↗ | (On Diff #17126) | Why do we need atomic? |
| 13 ↗ | (On Diff #17126) | In GlobalNetworkSingleton we designed this method so that it handles a task that accepts NetworkModule as an argument. The reason was that we should never be able to call any method on NetworkModule without using GlobalNetworkSingleton. I think it would be really beneficial to do the same for GlobalDBSingleton. So a task should accept DatabaseQueryExecutor as an argument - currently we're using DatabaseQueryExecutor which returns thread_local SQLiteQueryExecutor instance. That may cause more issues because when going from single to multiple threads on iOS, we will create new instance of SQLiteQueryExecutor. |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.cpp | ||
|---|---|---|
| 3 ↗ | (On Diff #17126) | Same as with <atomic> |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
| 5 ↗ | (On Diff #17126) | It was my oversight to forget to remove this import. We are not using it. |
| 13 ↗ | (On Diff #17126) | The biggest problem here is that we respond to rescinds and notifications with API implemented in MessageOperationsUtilities and ThreadOperationsUtilities. Those API call DatabaseManager.getQueryExecutor internally. We would need to refactor those APIs. Additionally once multithreading is enabled all tasks are using the same SQLiteQueryExecutor instance so the effect you described above is already achieved on Android and partially on iOS when we create second SQLiteQueryExecutor instance after enabling multithreading. But why do we consider it a problem to create new SQLiteQueryExecutor instance on iOS when enabling multithreading? My guess (it may be wrong) is that you are worried about the case when we instantiate SQLiteQueryExecutor instance (which calls migrate that touches database) at the same time there is another db task going on the main thread. If that is the case then further differentials in the stack take care so that such situation does not happen. |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
|---|---|---|
| 13 ↗ | (On Diff #17126) | I am not against introducing changes you suggested above. Just wanted to clarify that we do so to achieve better code quality and not as a safety measure. |
Comment Actions
We should be really sure about this code. If there are any issues, it will probably cause crashes we don't know about. I encourage everybody on this diff to be extremely thorough. We should be 100% sure of every line here. We should not be guessing, or trying things that seem to work without understanding 100% of why/how they work.
Comment Actions
I'll defer to tomek for approval, I don't see anything "wrong" with this though.
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.cpp | ||
|---|---|---|
| 12–16 ↗ | (On Diff #17098) | I think the goal is to minimize the complexity in any given code path. That being said, I treat it more like a rule-of-thumb rather than an absolute. I guess it's just a personal preference, as in most expression-oriented functional languages, an if statement has else as a mandatory branch. |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
|---|---|---|
| 13 ↗ | (On Diff #17126) |
For SQLite it might be ok, but for other dbs it might be an issue because we would keep resources associated with connections occupied. Main reason for hiding query executor behind global singleton is that we could use it in every context and it will just work. Also, using query executor without the singleton is likely to cause bugs. At this point we can keep the current solution, but refactoring it to somehow hide db API in the callback is an improvement. We can give either executor or operation utilities to the lambda. I think it would be a good followup task. |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
|---|---|---|
| 21 ↗ | (On Diff #17334) | It seems like multithreadingEnabled and databaseThread != nullptr should usually have the same value. The difference is that multithreadingEnabled is atomic - we needed a synchronized value because in D5233 we're accessing it from multiple threads. Using multithreadingEnabled wasn't enough because it is a unique pointer and it can't be made atomic. This approach is not thread safe because if we run enableMultithreadingCommonImpl from multiple threads we will have undefined behavior. This isn't an issue for us, because currently enableMultithreadingCommonImpl is called only from the main thread. But if we ever decide to change this, we will run into hard to debug bugs. So at this point, I think it will be better to use a shared, atomic pointer. It will be atomic so this whole function would become thread safe. Also reading its value in D5233 will be safe and efficient. Accepting this diff because we currently don't have a threading issue, but the solution is fragile and it is a really good idea to improve it. |
Comment Actions
Accepting this diff because we currently don't have a threading issue, but the solution is fragile and it is a really good idea to improve it.
@marcin, can you create a task for this and link it here before landing?
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
|---|---|---|
| 29 ↗ | (On Diff #17334) | Shouldn't this be private? |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
|---|---|---|
| 21 ↗ | (On Diff #17334) | There might be an issue with atomic shared pointer (context is in D5233) but we can use multithreadingEnabled variable instead of databaseThread != nullptr inside if statement to get rid of the issue described above without atomic shared pointer. |
| 29 ↗ | (On Diff #17334) | Definitely, will change it. |
| native/cpp/CommonCpp/NativeModules/InternalModules/GlobalDBSingleton.h | ||
|---|---|---|
| 21 ↗ | (On Diff #17334) |
I think I was too optimistic regarding this matter. To make it work we would need to atomically compare and exchange (it is possible) before we actually assign a WorkerThread pointer - I am suspicious about it. Since the code works correctly for now I will create a follow up Linear task to improve this line and land this diff. |