Skip to content
New issue

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

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

Already on GitHub? # to your account

Handle methodchannel calls asynchronously to avoid blocking the UI #272

Merged
merged 2 commits into from
Aug 3, 2021

Conversation

oxtoacart
Copy link

When finishing a voice memo, we've noticed that the loading indicator hangs while the voice memo gets encrypted. That's because we were handling the method channel calls on the main thread. This PR changes that so that they're handled on another thread.

While I was testing the VPN on/off button, I noticed that we were crashing with the below error.

08-02 16:13:17.323 30202 30202 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
08-02 16:13:17.323 30202 30202 F DEBUG   : Build fingerprint: 'samsung/beyond0qltesq/beyond0q:11/RP1A.200720.012/G970USQS5GUF1:user/release-keys'
08-02 16:13:17.323 30202 30202 F DEBUG   : Revision: '17'
08-02 16:13:17.323 30202 30202 F DEBUG   : ABI: 'arm64'
08-02 16:13:17.324 30202 30202 F DEBUG   : Timestamp: 2021-08-02 16:13:17-0500
08-02 16:13:17.324 30202 30202 F DEBUG   : pid: 24003, tid: 29583, name: Thread-13  >>> org.getlantern.lantern <<<
08-02 16:13:17.324 30202 30202 F DEBUG   : uid: 10672
08-02 16:13:17.324 30202 30202 F DEBUG   : signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
08-02 16:13:17.324 30202 30202 F DEBUG   : Abort message: 'fdsan: attempted to close file descriptor 132, expected to be unowned, actually owned by ParcelFileDescriptor 0x5bea8f0'

The Android VPNService has a TUN device which we use to capture IP packets. It turns out that we're not supposed to close that device ourselves when the VPN stops, but we have been doing that. I think my phone must have a gotten a recent OS update that enforces that rule. So, while I was in here, I stopped closing that file descriptor.

  • If you refactored existing code, have you tested the refactored functionality against the old version to make sure you didn't break anything?
  • Do the tests pass? Consistently?
  • Did this change improve test coverage?
  • Is the code in question being linted? If not, consider adding a linter step to CI. If yes, make sure the linter is happy.
  • Have you logged tickets for related technical debt with the label “techdebt”?

@oxtoacart oxtoacart force-pushed the ox/asyncmethodhandler branch from ddd2bfd to 5d5f0ec Compare August 2, 2021 21:35
override fun onMethodCall(call: MethodCall, result: MethodChannel.Result) {
final override fun onMethodCall(call: MethodCall, mcResult: MethodChannel.Result) {
// Process all calls on a background thread, then post results back on the main thread
asyncHandler.post {
Copy link
Author

Choose a reason for hiding this comment

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

This is the main fix - post work to an async handler, then post response back to mainHandler.

@oxtoacart oxtoacart force-pushed the ox/asyncmethodhandler branch from 5d5f0ec to c75cd5a Compare August 2, 2021 21:36
@oxtoacart oxtoacart changed the title Handle methdchannel calls asynchronously to avoid blocking the UI Handle methodchannel calls asynchronously to avoid blocking the UI Aug 2, 2021
@@ -26,7 +27,14 @@ abstract class BaseModel(
val db: DB,
) : EventChannel.StreamHandler, MethodChannel.MethodCallHandler {
private val activeSubscribers = ConcurrentSkipListSet<String>()
private val handler = Handler(Looper.getMainLooper())
protected val mainHandler = Handler(Looper.getMainLooper())
private val asyncHandlerThread = HandlerThread("BaseModel-AsyncHandler")
Copy link

Choose a reason for hiding this comment

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

Instead of using HandlerThread we can use a Coroutine for a more lightweight thread.
Something like this:

private var parentJob = Job()
    private val coroutineContext: CoroutineContext
        get() = parentJob + Dispatchers.Main
    private val scope = CoroutineScope(coroutineContext)

Copy link

Choose a reason for hiding this comment

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

including also that we avoid the need to keep initializing the Handler in order to keep working

private val asyncHandlerThread = HandlerThread("BaseModel-AsyncHandler")

init {
asyncHandlerThread.start()
Copy link

Choose a reason for hiding this comment

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

Im not sure If we should have the HandlerThread always listening or in this case looping even when is not being called.

@Crdzbird
Copy link

Crdzbird commented Aug 2, 2021

I did a little time comparisons between the Coroutines against HandlerThread

This image show the time that requires in order to complete the sentence using HandlerThread
Screen Shot 2021-08-02 at 17 12 11

On this one display the same sentence however using a Coroutine on a parallel thread.
Screen Shot 2021-08-02 at 17 01 08

It can be a good implementation, but the pr works fine :)

@oxtoacart
Copy link
Author

Thanks for the review @Crdzbird ! I'm going to stick with the Thread implementation since we use Threads already and the last time I tried introducing coroutines, I ended up with some surprising and difficult to understand bugs :)

@oxtoacart oxtoacart merged commit 30e9050 into main Aug 3, 2021
@oxtoacart oxtoacart deleted the ox/asyncmethodhandler branch August 3, 2021 00:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants