Unrelated to this diff, but wondering: why does Android initiate multithreadingEnabled(true) but iOS does multithreadingEnabled(false)?

In D5576#165367, @ashoat wrote:

Unrelated to this diff, but wondering: why does Android initiate multithreadingEnabled(true) but iOS does multithreadingEnabled(false)?

I had discussion about this with @tomek at the initial stage of work on GlobalDBSingleton. Before introduction of GlobalDBSingleton application created a dedicated database thread when CommCoreModule was created - when application foregrounded and started JavaScript thread. GlobalDBSingleton is meant to provide single threaded access to database not only when it is foregrounded but also in the background in response to notifications. One idea was to create dedicated database thread from the beginning of application existence. This would mean that if the application is started by the system to respond to notification we would create new thread (dedicated database thread). We didn't see it to be an issue on Android, however I was told that it might be an issue on iOS. Apple might detect that we create additional thread (use more resources) to respond to notification and send them less frequently. Therefore we decided, that GlobalDBSingleton will create dedicated database thread from the beginning on Android and only after JavaScript starts (CommCoreModule is created) on iOS. On iOS before JavaScript starts every database task is asynchronously dispatched on the application's main thread queue (main thread will execute it).

Thanks for explaining, @marcin!

Haven't worked with std::atomic before so will need to do some reading up before reviewing this... but based on the C++ book I've been using as a reference:

This chapter doesn’t discuss std::atomic or how to devise your own lock- free solutions in detail, because it’s incredibly difficult to do correctly and is best left to experts. However, in simple situations, such as in Listing 19-10, you can employ a std::atomic to make sure that the increment or decrement operations cannot be divided.

Unless you have a very simple concurrent access problem, such as the one in this section, you really shouldn’t try to implement lock-free solutions on your own. Refer to the Boost Lockfree library for high-quality, thoroughly tested lock-free containers. As always, you must decide whether a lock-based or lock-free implementation is optimal.

So we should probably be very careful with whatever we do here.

Is there a reason we can't use std::mutex and std::lock_guard instead? That's a pattern we use elsewhere in the codebase and more importantly std::mutex + lock-based synchronization is something I understand in much more depth than lock-free concurrency...

In D5576#166868, @atul wrote:

Is there a reason we can't use std::mutex and std::lock_guard instead? That's a pattern we use elsewhere in the codebase and more importantly std::mutex + lock-based synchronization is something I understand in much more depth than lock-free concurrency...

Honestly, I think that mutexes are more complicated and error-prone than atomic variables. Designing a lock-free solution is indeed complicated, but in our case we don't need synchronization, but only visibility so it's a lot easier. Using mutexes will be huge overkill when the only thing we need is for one thread to see an update that other thread might have done. So basically, our goal is to avoid data races https://en.cppreference.com/w/cpp/language/memory_model.

In D5576#166970, @tomek wrote:

In D5576#166868, @atul wrote:

Is there a reason we can't use std::mutex and std::lock_guard instead? That's a pattern we use elsewhere in the codebase and more importantly std::mutex + lock-based synchronization is something I understand in much more depth than lock-free concurrency...

Honestly, I think that mutexes are more complicated and error-prone than atomic variables. Designing a lock-free solution is indeed complicated, but in our case we don't need synchronization, but only visibility so it's a lot easier. Using mutexes will be huge overkill when the only thing we need is for one thread to see an update that other thread might have done. So basically, our goal is to avoid data races https://en.cppreference.com/w/cpp/language/memory_model.

@tomek, you're not wrong in a general sense... yes, mutexes are complicated. But did you read @atul's inline comment, though? It seems pretty clear that he is right, and the current design is simply insufficient for what we need to do.

I've discussed this change with @tomek but sharing what we established.

@atul and @ashoat fairly pointed problem with the previous solution, here is the vulnerable scenario:

1. there are some tasks in the queue scheduled previously
2. two clearSensitiveDatas are scheduled simultaneously (note that there is no possibility to execute those at the same time but scheduling at the same time is possible), and flag tasksCancelled is set to true because setting flag takes place before actual task execution
3. tasks that were in the queue are canceled
4. first clearSensitiveData execute and set tasksCancelled to false
5. second clearSensitiveData execute but the flag is set to false which means another tasks can be added to a queue which should be avoided.

to avoid this the flag is not set to false after task execution directly but setting the flag is scheduled as another non-cancellable task which means it will set the flag after execution of second`clearSensitiveData` in the previous example.

But still, there's a bug here: we should've called GlobalDBSingleton::instance.setTasksCancelled(true); inside the scheduled task.

we don not consider this as bug, what is more it will cause that tasks already scheduled in queue will not be cancelled. We can add this additionally but I don't think there is a need to do it.