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

Make cryptoDevice calls suspendable #8485

Merged
merged 6 commits into from
Jun 1, 2023
Merged

Conversation

BillCarsonFr
Copy link
Member

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fixes #8482

getCryptoDevice calls in rust are using runBlocking, it was causing ANRs. These calls are now suspendable, updated the code accordingly.

Additional change in ReadService on getHomeServerCapabilities() that was also causing ANRs.

Motivation and context

Screenshots / GIFs

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@BillCarsonFr BillCarsonFr requested a review from bmarty May 30, 2023 21:50
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, some small remarks.

it.deviceId != session.sessionParams.deviceId
}
setState { copy(hasAnyOtherSession = Loading()) }
viewModelScope.launch(Dispatchers.IO) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not mandatory to provide a dispatcher here, getCryptoDeviceInfo will switch to this dispatcher using withContext.

pushersManager.enqueueRegisterPusher(pushKey, gateway)
runBlocking {
pushersManager.enqueueRegisterPusher(pushKey, gateway)
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using runBlocking, I think we should use runTest, so update the method declaration like this:

fun `when enqueueRegisterPusher, then HttpPusher created and enqueued`() = runTest {

@@ -32,7 +33,7 @@ class DefaultGetDeviceInfoUseCaseTest {

@Test
fun `when execute, then get crypto device info`() {
val result = getDeviceInfoUseCase.execute()
val result = runBlocking { getDeviceInfoUseCase.execute() }
Copy link
Member

Choose a reason for hiding this comment

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

use runTest instead of runBlocking:

fun `when execute, then get crypto device info`() = runTest {

@BillCarsonFr BillCarsonFr force-pushed the feature/bca/fix_anrs branch from 3ea8b63 to 7e6376b Compare May 31, 2023 13:16
@BillCarsonFr BillCarsonFr added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label May 31, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 1, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@BillCarsonFr BillCarsonFr merged commit 2f1a7b7 into develop Jun 1, 2023
@BillCarsonFr BillCarsonFr deleted the feature/bca/fix_anrs branch June 1, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sentry ANR in SelfVerificationViewModel init
2 participants