Implements GlobalDBSingleton API for both platforms. On iOS we use main queue and atomic boolean for synchronization. On Android we enable multithreading from the begining.
Details
Details
Spawn multiple threads in AppDelegate (for instance multiple instances of WorkerThread) and schedule database tasks from them using GlobalDBSingletonIOSProxy API. Do it before and after CommCoreModule instantiation. No crash should happen.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/db-singleton
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/ios/Comm/GlobalDBSingletonIOSProxy.mm | ||
|---|---|---|
| 13 | is there a reason we can't use dispatch_async? I'm assuming we don't have a hard timeline for tasks. | |
| native/ios/Comm/GlobalDBSingletonIOSProxy.mm | ||
|---|---|---|
| 13 | You are right. scheduleOrRun method signature suggests that we do not make guarantees about the time a task will be executed. If someone is interested in waiting for the task completion they can put a promise inside a task and wait on it in a thread that is calling this method. | |
Comment Actions
The code looks rather ok - only not sure about enableMultithreading safety.
Additional note: I was wondering if this code structure can be improved by taking a similar approach to what we do e.g. for Logger. We have a common interface there with platform-specific implementations. I guess it might be a big improvement, because currently it would be hard to call GlobalDBSingletonIOSProxy on iOS from C++ code.
| native/ios/Comm/GlobalDBSingletonIOSProxy.mm | ||
|---|---|---|
| 19–26 ↗ | (On Diff #17127) | What happens when we have a task scheduled using scheduleOrRun on dispatch_get_main_queue and then main thread calls enableMultithreading before the queue started processing the task? |
Comment Actions
I get your point and can surely implement that. But is it likely that we call this class from C++? We generally try to avoid platform depended code in CommonCpp. The only place in CommonCpp we call GlobalDBSingleton is CommCoreModule which does not need synchronization implemented by this class.
| native/ios/Comm/GlobalDBSingletonIOSProxy.mm | ||
|---|---|---|
| 19–26 ↗ | (On Diff #17127) | This is one thread so enableMultithreading completes and when block with scheduleOrRun starts being executed the task will be run on dedicated database thread. I found an article https://www.objc.io/issues/2-concurrency/low-level-concurrency-apis/. It has a sentence: " If you're submitting a block to GCD, and that code blocks the thread, this thread is no longer available at that point in time to do other work -- it's blocked". This leads to conclusion that blocks submitted to the queue are executed atomically. So it is not possible that queue starts processing block with scheduleOrRun, comes to a point it wants to call task() synchronously (since multithreading was not enabled), then stops and goes to a code that enables multithreading and comes back to executing task at the main thread afterwards. |
Comment Actions
After considering this idea for a while I came to the conclusion that we already do so for Android. We define JNI helper classes in headers in CommonCpp and implement them in Android specific directory. We can do the same for iOS - define iOS proxy class in header in CommonCpp and implement it in Objective C++ file in iOS specific directory.
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
Why it doesn't need this synchronization? Is calling CommCoreModule methods from App Delegate safe? I think that the safest approach would be to call methods on GlobalDBSingleton in both App Delegate and CommCoreModule. These methods would have different implementation for different platforms. This approach would reduce probability of mistakes. Can you see any disadvantages?
| native/ios/Comm/GlobalDBSingletonIOSProxy.mm | ||
|---|---|---|
| 19–26 ↗ | (On Diff #17127) | Ok, this makes sense. I tried to find any issue in the solution, but it seems to be quite solid. Basically, every operation that involves using GlobalDBSingleton happens on main thread. That allows doing the operations without any synchronization. The only thing which we have to ensure is that CommCoreModule also uses GlobalDBSingleton from main thread. I think that if we don't do that, there might be some threading bugs. |
Comment Actions
Why it doesn't need this synchronization? Is calling CommCoreModule methods from App Delegate safe?
We don't call CommCoreModule from AppDelegate. I was talking about using GlobalDBSingleton methods in CommCoreModule. The reason it safe is that we enable multithreading in CommCoreModule constructor.
I think that the safest approach would be to call methods on GlobalDBSingleton in both App Delegate and CommCoreModule. These methods would have different implementation for different platforms. This approach would reduce probability of mistakes. Can you see any disadvantages?
I agree, but my initial thought was to have a common interface for both platforms. But since we end up implementing additional synchronization layers then perhaps it is better to define header in CommonCpp and implement independently for Android and iOS.
| native/ios/Comm/GlobalDBSingleton.mm | ||
|---|---|---|
| 12–19 ↗ | (On Diff #17341) | Could you remind me why we're scheduling only from the main thread? I think that currently the code is correct, but it might be possible to simplify it. Does introducing a shared atomic pointer solve the causes? |
| native/ios/Comm/GlobalDBSingleton.mm | ||
|---|---|---|
| 12–19 ↗ | (On Diff #17341) |
We could create another atomic boolean that indicates whether there is ongoing database operation that started its execution before enableMultithreading and schedule enableMultithreading execution on the main thread only if it is true. This solution is more general but also more complicated so I would personally defer its implementation until we really need it. Moreover I have probably been to optimistic regarding atomic shared pointer during my private discussion with @tomek . From the docs it looks like they are C++20 feature (https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic2). I will try to compile the code to see if we can use it the same way we use if constexpr which is a C++17 feature. |
| native/ios/Comm/GlobalDBSingleton.mm | ||
|---|---|---|
| 12–19 ↗ | (On Diff #17341) | I was unable to compile std::atomic<std::shared_ptr<>> here. |
Comment Actions
We must use sync since we call this from CommCoreModule now
Can you provide more context on this last-minute change?
Comment Actions
Unfortunately it was a mistake (not a harmful one) I made being under pressure. I was looking at the entire code once again before landing and created a scenario in my mind that some thread calls enableMultithreading , then escapes from a function before a task completes (it is an asynchronous dispatch). I thought that another thread might then schedule a task before a database thread is created so I decided to make it synchronous. This is ridiculous since enableMultithreadingCommonImpl sets atomic boolean flag only after database thread is created. Before this flag is set every task will execute on main thread. Only after it is set tasks will being to execute on dedicated database thread. I was under pressure and my thinking was not clear. This change was unnecessary but not harmful. I will create a follow-up task for database singleton issue (mainly to resolve whether we can use atomic shared pointers as Tomek suggested) and I will make sure that it has a subtask to make this call asynchronous back. I will also make sure release build that includes this stack has asynchronous call here.