Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move database operation off main thread #4053

Merged
merged 11 commits into from Oct 28, 2019
Merged

move database operation off main thread #4053

merged 11 commits into from Oct 28, 2019

Conversation

@chliangGoogle
Copy link
Contributor

@chliangGoogle chliangGoogle commented Oct 11, 2019

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.

@chliangGoogle
Copy link
Contributor Author

@chliangGoogle chliangGoogle commented Oct 15, 2019

This is not going to the incoming release. So no time urgency.

@chliangGoogle
Copy link
Contributor Author

@chliangGoogle chliangGoogle commented Oct 16, 2019

breakage at the podspec which is strange cuz I didn't update the podspec file.

@paulb777
Copy link
Member

@paulb777 paulb777 commented Oct 16, 2019

It's probably a build or test failure that fails to log. Run ./scripts/pod_lib_lint.rb --verbose FirebaseMessaging.podspec to diagnose.

@chliangGoogle
Copy link
Contributor Author

@chliangGoogle chliangGoogle commented Oct 16, 2019

I ran
./scripts/pod_lib_lint.rb FirebaseMessaging.podspec --platforms=ios --verbose
and this is the output:

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.

@chliangGoogle chliangGoogle requested a review from morganchen12 Oct 25, 2019
@chliangGoogle
Copy link
Contributor Author

@chliangGoogle chliangGoogle commented Oct 25, 2019

Looks like more stall is detected at b/143297145, the travis is green now and please take a look at.

@chliangGoogle chliangGoogle requested a review from ryanwilson Oct 25, 2019
Copy link
Contributor

@morganchen12 morganchen12 left a comment

LGTM, but please wait for Core approval before merging.

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

LGTM
Sorry, I don't have much context, so could provide only a shallow review.

}
sqlite3_finalize(statement);
__block NSMutableArray *rmqIDArray = [NSMutableArray array];
dispatch_sync(_databaseOperationQueue, ^{
Copy link
Contributor

@maksymmalyhin maksymmalyhin Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@chliangGoogle chliangGoogle Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@maksymmalyhin maksymmalyhin Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yaeh, I agree and completely understand. Just would like to share our experience because we had issue with dispatch_sync just recently in GDT

@chliangGoogle chliangGoogle merged commit 67de928 into master Oct 28, 2019
2 checks passed
@chliangGoogle chliangGoogle deleted the oct-fcm-rmq branch Oct 28, 2019
@chliangGoogle chliangGoogle added this to the M59 milestone Oct 28, 2019
@firebase firebase locked and limited conversation to collaborators Nov 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants