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

Implemented background platform channels #830

Merged

Conversation

gaaclarke
Copy link
Contributor

@gaaclarke gaaclarke commented Jun 27, 2022

fixes #719

This makes all the channel calls happen on thread pool instead of executing on the platform thread. It is still a bit inefficient since technically we could be executing the sqlite commands on the thread pool but I had some difficultly implementing it on Android. Sqlite would complain about getting a connection for all the threads in the thread pool.

This is still an improvement. With flutter's customer money we are seeing sqlite requests happening at startup when the platform thread is very busy so the sqlite requests are being throttled by the platform thread when they have no need to execute on the platform thread. Especially on Android.

Don't let the small change fool the reviewer. This is a large change in terms of functionality. I've looked through the code and ran the tests and I think the change is safe.

thread to using the plugin thread pool to do so.
@@ -60,6 +60,6 @@ dependencies {
androidTestImplementation 'androidx.test.ext:junit:1.1.2'
androidTestImplementation 'androidx.test:runner:1.3.0'
androidTestImplementation 'androidx.test:runner:1.3.0'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.3.0'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we want to check this in. I had to make this change locally to fix gradle dependency resolution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I try to use the latest version as much as possible too!

@alextekartik
Copy link
Contributor

Thanks again for following flutter evolutions on this plugin. Indeed SQLite has some limitation on threads that might not match the flutter thread pool. For example, in a transaction, all commands must happen in the same thread so I think we still have to run in a dedicated thread on the native side (I might be wrong though, don't take anything for granted), at least on Android ((i.e. if a query happen in a different thread than the one that called BEGIN TRANSACTION, we could have deadlock). I will merge and make some additional testing on my side. Thanks again!

@alextekartik alextekartik merged commit 7f953b9 into tekartik:master Jun 27, 2022
@alextekartik
Copy link
Contributor

I have published version 2.0.3-dev.4 with your changes. I have only tested on Android.
I will likely publish it as 2.1.0 stable next week if everything looks fine. I really appreciate your support.

@gaaclarke
Copy link
Contributor Author

It's my pleasure. Just to warn you, I'll be in the office one more day (6/28) than I'm out for vacation for a couple of weeks. So if you need something I can take a look tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to using Android Background Platform Channels
2 participants