move database operation off main thread #4053
Conversation
… weird and fix the unit tests
charlotteliang
requested review from
paulb777,
morganchen12 and
maksymmalyhin
|
This is not going to the incoming release. So no time urgency. |
|
breakage at the podspec which is strange cuz I didn't update the podspec file. |
|
It's probably a build or test failure that fails to log. Run |
|
I ran bundle exec pod lib lint --sources=https://cdn.cocoapods.org/ --include-podspecs={FirebaseAnalyticsInterop.podspec,FirebaseCore.podspec,GoogleUtilities.podspec,FirebaseCoreDiagnosticsInterop.podspec,FirebaseCoreDiagnostics.podspec,GoogleDataTransportCCTSupport.podspec,GoogleDataTransport.podspec,FirebaseInstanceID.podspec} FirebaseMessaging.podspec --platforms=ios --verbose Doesn't seem to show any errors or general logs. |
|
Looks like more stall is detected at b/143297145, the travis is green now and please take a look at. |
| } | ||
| sqlite3_finalize(statement); | ||
| __block NSMutableArray *rmqIDArray = [NSMutableArray array]; | ||
| dispatch_sync(_databaseOperationQueue, ^{ |
There was a problem hiding this comment.
Ideally, we should not use dispatch_sync unless it is really necessary because it is pretty easy to loose track of all consequent calls and call e.g. unackedS2dRmqIds from _databaseOperationQueue which will lead to a deadlock.
There was a problem hiding this comment.
We def don't call a dispatch_sync call inside the queue, unackedS2dRmqIds is only called from outside. This class structure is quite simple that most of the writes are async and read is sync and can be ensure it's not called from a queue either.
Some of the read operations of database can not be async for now because either the public API is a sync call that calling this or we need to load data from database after app start right away because some other operation needs to access the data right away and a delay could result it thinks there's no such data.
The most painful reason is that the original structure of this SDK is poorly designed for everything to be blocking call, so it will take a huge refactor to eventually move everything off main thread.
At this point it's still better to first move some of the operations off main thread, than everything on main thread.
There was a problem hiding this comment.
Yaeh, I agree and completely understand. Just would like to share our experience because we had issue with dispatch_sync just recently in GDT
b/142357385 People find the app stuck on RMQ database write operation as they are on main thread.
Moving all the write operations off main thread.
Some of the sync methods used to return a value, it is used to tell the operation succeed and print error if not. We will remove those return value and print error inside the method.
There are other return values are not used at all by the parent class. Remove those and make them async method.
Also refactor the method scanWithRmqMessageHandler: which is very interesting. It has two callbacks as parameters, each is used on different occasions. This function reads data from database, instead of return the results at once, it triggers callback per row of database read. This used to work because database operation is blocking, so client just blocking read each row of database and return a callback to continue the next step. Unified both callback to one and trigger the callback after database read is complete and return with the database results.