-
-
Notifications
You must be signed in to change notification settings - Fork 519
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
Remove a thread blocking behaviour #547
Conversation
Make decryption asynchronous
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 :) |
Makes sense, I'll have an Android specialist on my team to look at it! 👍 |
There was a problem hiding this 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.
@@ -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( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -112,30 +113,25 @@ public DecryptionResult decrypt(@NonNull final String alias, | |||
final byte[] decryptedUsername = crypto.decrypt(username, usernameEntity); |
There was a problem hiding this comment.
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
?
@savelichalex Do you know if it could this fix #525 too? |
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":
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!