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.

Sign up for GitHub

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

Jump to bottom

Remove a thread blocking behaviour #547

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove a thread blocking behaviour #547

wants to merge 2 commits into from

Conversation

savelichalex
Copy link

Hi guys.
Thanks for your awesome library, we've been using it for a long time, so I want to contribute in return.

Recently we encountered a lot of crashes. Though it's not actual crashes as I found out, this is when a system close the app, because it didn't response for a while (a thread didn't get unlocked). There is an example of such a "crash":

io.sentry.android.core.ApplicationNotResponding: Application Not Responding for at least 5000 ms.
    at sun.misc.Unsafe.park(Unsafe.java)
    at java.util.concurrent.locks.LockSupport.park(LockSupport.java:190)
    at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:868)
    at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedInterruptibly(AbstractQueuedSynchronizer.java:1023)
    at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly(AbstractQueuedSynchronizer.java:1334)
    at java.util.concurrent.Semaphore.acquire(Semaphore.java:318)
    at com.swmansion.reanimated.NodesManager.performOperations(NodesManager.java:251)
    at com.swmansion.reanimated.NodesManager.onAnimationFrame(NodesManager.java:280)
    at com.swmansion.reanimated.NodesManager.access$000(NodesManager.java:65)
    at com.swmansion.reanimated.NodesManager$1.doFrameGuarded(NodesManager.java:170)
    at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29)
    at com.facebook.react.modules.core.ReactChoreographer$ReactChoreographerDispatcher.doFrame(ReactChoreographer.java:175)
    at com.facebook.react.modules.core.ChoreographerCompat$FrameCallback$1.doFrame(ChoreographerCompat.java:85)
    at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1140)
    at android.view.Choreographer.doCallbacks(Choreographer.java:946)
    at android.view.Choreographer.doFrame(Choreographer.java:870)
    at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1127)
    at android.os.Handler.handleCallback(Handler.java:938)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:210)
    at android.os.Looper.loop(Looper.java:299)
    at android.app.ActivityThread.main(ActivityThread.java:8105)
    at java.lang.reflect.Method.invoke(Method.java)

IMO it looks like a deadlock. I tried to trace it without any luck, I tried to stop any other activity (animations) while keychain is operating, also didn't help. So I decided to untie the deadlock in a place that I'm sure is involved in it - thread block.
Correct me if I wrong, but as far as I understand the necessity of it, is that API just wasn't designed to be asynchronous, and to prevent concurrent calls to biometry API thread block was introduced. So I tried to untackle this and make it concurrent, at the same time keeping serial access to biometry calls.

It isn't final solution, though we already shipped it to our beta users, and they confirmed that the crash is gone. I would like to fix all tests, though as it isn't fast, I would like to receive your feedback on that, as it seems like a very big change, and I would like to reach a consensus on internal APIs first. Looking forward for it!

@oblador
Copy link
Owner

oblador commented Jun 23, 2022

Thanks a bunch for this PR, I'm noticing tests are failing, would you mind having a look at that?

@savelichalex
Copy link
Author

Thanks a bunch for this PR, I'm noticing tests are failing, would you mind having a look at that?

Hi @oblador 👋 As I already mentioned, I would like to fix them, but at first I would like to get some feedback about approach I took (internal API design wise), as if it doesn't look good for you, it'll be required to rewrite tests completely. Once again, I would be happy to fix them, when overall design approach is confirmed to be OK. Thanks for understanding :)

@oblador
Copy link
Owner

oblador commented Jun 23, 2022

Makes sense, I'll have an Android specialist on my team to look at it! 👍

androideveloper
androideveloper reviewed Jun 24, 2022
View reviewed changes
Copy link
Collaborator

@androideveloper androideveloper left a comment

Choose a reason for hiding this comment

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

Hi @savelichalex . Thanks for the PR!
I added a couple of comments below. API wise looks good, the main concern is the java API used here, which are not available on lower android versions.

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageFacebookConceal.java
@@ -112,30 +113,25 @@ public DecryptionResult decrypt(@NonNull final String alias,
final byte[] decryptedUsername = crypto.decrypt(username, usernameEntity);
final byte[] decryptedPassword = crypto.decrypt(password, passwordEntity);

return new DecryptionResult(
return CompletableFuture.supplyAsync(() -> new DecryptionResult(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This call requires API level 24. (CompletableFuture was introduced in 24.)
Current minimum is 21. There is no desugaring for this API to support older versions of android, which means bumping min level => bumping min level for all apps using this sdk. Any alternative? Rx and kotlin come to my mind.

Copy link
Author

Choose a reason for hiding this comment

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

What do you think to include CompletableFuture compat version? Like what is suggested here: https://stackoverflow.com/a/38375991 , or maybe I can try to write my own wrapper (because it seems that multithreading that CompletableFuture provide is not that important for the task)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This compat version seems to be not official one. We will have maintenance issues later on.

Copy link

Choose a reason for hiding this comment

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

@savelichalex We tested this patch and observed it fixes the crash issue, writing own wrapper sounds good to me will you be able to update this pr.

Copy link
Author

Choose a reason for hiding this comment

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

@ankitsingh08 when I tried to make the wrapper turned out it's not a 20 minutes ride :D I have an idea to make it with callbacks, unfortunately I don't have much time for this right now, but I'll try to allocate some time for it

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageFacebookConceal.java
@@ -112,30 +113,25 @@ public DecryptionResult decrypt(@NonNull final String alias,
final byte[] decryptedUsername = crypto.decrypt(username, usernameEntity);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't these 2 decrypt calls also be part of supplyAsync?

@kabirbaidhya
Copy link

@savelichalex Do you know if it could this fix #525 too?

@perunt perunt mentioned this pull request Sep 27, 2022
Merged
7 tasks
@frw frw mentioned this pull request Nov 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@androideveloper androideveloper androideveloper left review comments

@ankitsingh08 ankitsingh08 ankitsingh08 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@savelichalex @oblador @kabirbaidhya @androideveloper @ankitsingh08 @sertony