Skip to content

Convert AccountService to suspend functions #2354

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

Merged
merged 4 commits into from
Nov 10, 2020
Merged

Convert AccountService to suspend functions #2354

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
35768ff
Convert `AccountService` to suspend functions
Dominaezzz Nov 6, 2020
983e028
Remove incorrect comment
Dominaezzz Nov 6, 2020
0da4ff7
Run ktlint
Dominaezzz Nov 6, 2020
6207aab
Remove cancellation handling
Dominaezzz Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/ChangePasswordTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package org.matrix.android.sdk.account

import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.matrix.android.sdk.InstrumentedTest
import org.matrix.android.sdk.api.failure.isInvalidPassword
import org.matrix.android.sdk.common.CommonTestHelper
Expand Down Expand Up @@ -43,8 +45,8 @@ class ChangePasswordTest : InstrumentedTest {
val session = commonTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(withInitialSync = false))

// Change password
commonTestHelper.doSync<Unit> {
session.changePassword(TestConstants.PASSWORD, NEW_PASSWORD, it)
commonTestHelper.runBlockingTest {
session.changePassword(TestConstants.PASSWORD, NEW_PASSWORD)
}

// Try to login with the previous password, it will fail
Expand Down
4 changes: 2 additions & 2 deletions ...-sdk-android/src/androidTest/java/org/matrix/android/sdk/account/DeactivateAccountTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ class DeactivateAccountTest : InstrumentedTest {
val session = commonTestHelper.createAccount(TestConstants.USER_ALICE, SessionTestParams(withInitialSync = false))

// Deactivate the account
commonTestHelper.doSync<Unit> {
session.deactivateAccount(TestConstants.PASSWORD, false, it)
commonTestHelper.runBlockingTest {
session.deactivateAccount(TestConstants.PASSWORD, false)
}

// Try to login on the previous account, it will fail (M_USER_DEACTIVATED)
Expand Down
10 changes: 10 additions & 0 deletions matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import kotlinx.coroutines.GlobalScope
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withTimeout
import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotNull
import org.junit.Assert.assertTrue
Expand Down Expand Up @@ -343,6 +344,15 @@ class CommonTestHelper(context: Context) {
await(latch, timeout)
}

// Transform a method with a MatrixCallback to a synchronous method
fun <T> runBlockingTest(timeout: Long = TestConstants.timeOutMillis, block: suspend () -> T): T {
return runBlocking {
withTimeout(timeout) {
block()
}
}
}

// Transform a method with a MatrixCallback to a synchronous method
inline fun <reified T> doSync(timeout: Long? = TestConstants.timeOutMillis, block: (MatrixCallback<T>) -> Unit): T {
val lock = CountDownLatch(1)
Expand Down
6 changes: 2 additions & 4 deletions ...ix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/account/AccountService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package org.matrix.android.sdk.api.session.account

import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.util.Cancelable

/**
* This interface defines methods to manage the account. It's implemented at the session level.
Expand All @@ -28,7 +26,7 @@ interface AccountService {
* @param password Current password.
* @param newPassword New password
*/
fun changePassword(password: String, newPassword: String, callback: MatrixCallback<Unit>): Cancelable
suspend fun changePassword(password: String, newPassword: String)

/**
* Deactivate the account.
Expand All @@ -46,5 +44,5 @@ interface AccountService {
* @param eraseAllData set to true to forget all messages that have been sent. Warning: this will cause future users to see
* an incomplete view of conversations
*/
fun deactivateAccount(password: String, eraseAllData: Boolean, callback: MatrixCallback<Unit>): Cancelable
suspend fun deactivateAccount(password: String, eraseAllData: Boolean)
}
23 changes: 5 additions & 18 deletions ...id/src/main/java/org/matrix/android/sdk/internal/session/account/DefaultAccountService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,17 @@

package org.matrix.android.sdk.internal.session.account

import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.session.account.AccountService
import org.matrix.android.sdk.api.util.Cancelable
import org.matrix.android.sdk.internal.task.TaskExecutor
import org.matrix.android.sdk.internal.task.configureWith
import javax.inject.Inject

internal class DefaultAccountService @Inject constructor(private val changePasswordTask: ChangePasswordTask,
private val deactivateAccountTask: DeactivateAccountTask,
private val taskExecutor: TaskExecutor) : AccountService {
private val deactivateAccountTask: DeactivateAccountTask) : AccountService {

override fun changePassword(password: String, newPassword: String, callback: MatrixCallback<Unit>): Cancelable {
return changePasswordTask
.configureWith(ChangePasswordTask.Params(password, newPassword)) {
this.callback = callback
}
.executeBy(taskExecutor)
override suspend fun changePassword(password: String, newPassword: String) {
changePasswordTask.execute(ChangePasswordTask.Params(password, newPassword))
}

override fun deactivateAccount(password: String, eraseAllData: Boolean, callback: MatrixCallback<Unit>): Cancelable {
return deactivateAccountTask
.configureWith(DeactivateAccountTask.Params(password, eraseAllData)) {
this.callback = callback
}
.executeBy(taskExecutor)
override suspend fun deactivateAccount(password: String, eraseAllData: Boolean) {
deactivateAccountTask.execute(DeactivateAccountTask.Params(password, eraseAllData))
}
}
28 changes: 13 additions & 15 deletions vector/src/main/java/im/vector/app/features/settings/VectorSettingsGeneralFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import android.widget.ImageView
import android.widget.Toast
import androidx.appcompat.app.AlertDialog
import androidx.core.view.isVisible
import androidx.lifecycle.lifecycleScope
import androidx.preference.EditTextPreference
import androidx.preference.Preference
import androidx.preference.PreferenceCategory
Expand Down Expand Up @@ -451,28 +452,25 @@ class VectorSettingsGeneralFragment @Inject constructor(
val newPwd = newPasswordText.text.toString()

showPasswordLoadingView(true)
session.changePassword(oldPwd, newPwd, object : MatrixCallback<Unit> {
override fun onSuccess(data: Unit) {
if (!isAdded) {
return
}
showPasswordLoadingView(false)
lifecycleScope.launch {
val result = runCatching {
session.changePassword(oldPwd, newPwd)
}
if (!isAdded) {
return@launch
}
showPasswordLoadingView(false)
result.fold({
dialog.dismiss()
activity.toast(R.string.settings_password_updated)
}

override fun onFailure(failure: Throwable) {
if (!isAdded) {
return
}
showPasswordLoadingView(false)
}, { failure ->
if (failure.isInvalidPassword()) {
oldPasswordTil.error = getString(R.string.settings_fail_to_update_password_invalid_current_password)
} else {
oldPasswordTil.error = getString(R.string.settings_fail_to_update_password)
}
}
})
})
}
}
}
dialog.show()
Expand Down
23 changes: 15 additions & 8 deletions ...n/java/im/vector/app/features/settings/account/deactivation/DeactivateAccountViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package im.vector.app.features.settings.account.deactivation

import androidx.lifecycle.viewModelScope
import com.airbnb.mvrx.FragmentViewModelContext
import com.airbnb.mvrx.MvRxState
import com.airbnb.mvrx.MvRxViewModelFactory
Expand All @@ -24,9 +25,12 @@ import com.squareup.inject.assisted.AssistedInject
import im.vector.app.core.extensions.exhaustive
import im.vector.app.core.platform.VectorViewModel
import im.vector.app.core.platform.VectorViewModelAction
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.launch
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.failure.isInvalidPassword
import org.matrix.android.sdk.api.session.Session
import java.lang.Exception

data class DeactivateAccountViewState(
val passwordShown: Boolean = false
Expand Down Expand Up @@ -67,19 +71,22 @@ class DeactivateAccountViewModel @AssistedInject constructor(@Assisted private v

_viewEvents.post(DeactivateAccountViewEvents.Loading())

session.deactivateAccount(action.password, action.eraseAllData, object : MatrixCallback<Unit> {
override fun onSuccess(data: Unit) {
_viewEvents.post(DeactivateAccountViewEvents.Done)
}
viewModelScope.launch {
val event = try {
session.deactivateAccount(action.password, action.eraseAllData)
DeactivateAccountViewEvents.Done
} catch (failure: Exception) {
if (failure is CancellationException) throw failure
Copy link
Member

Choose a reason for hiding this comment

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

Why CancellationException is treated apart, and why throwing? Will it make the app crash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CancellationException in general won't make the app crash.
When launch gets a CancellationException, it just gracefully stops executing.
When async gets a CancellationException, it just gracefully stops executing and other coroutines calling await() will get the cancellation exception.

The only case when it may crash the app is if it is directly thrown inside runBlocking, since it has to run in non-coroutine code.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the answer!

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it ok to let the CancellationException go to OtherFailure then? it won't change anything in the execution flow, but we'll loose the error in the UI if not posting to viewEvents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's technically not an error, the task has just been cancelled and this will only happen if the view model has been cleared, so if the UI has been destroyed.
But yeah it doesn't make much of a difference I can remove the special casing.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I wasn't thinking about this specific case, but more generally. We can get a cancellation if some suspend got cancelled by any inner stuff. By the way, we still have to be able to cancel all session coroutine (clearing cache/logout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That complicates things..... Is that the only time when you cancel all sessions coroutines?
In the case of viewModelScope the coroutine would already have been cancelled before user logs out or clears the cache, no?

Copy link
Member

Choose a reason for hiding this comment

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

I guess for most of the cases it will be ok yeah


override fun onFailure(failure: Throwable) {
if (failure.isInvalidPassword()) {
_viewEvents.post(DeactivateAccountViewEvents.InvalidPassword)
DeactivateAccountViewEvents.InvalidPassword
} else {
_viewEvents.post(DeactivateAccountViewEvents.OtherFailure(failure))
DeactivateAccountViewEvents.OtherFailure(failure)
}
}
})

_viewEvents.post(event)
}
}

companion object : MvRxViewModelFactory<DeactivateAccountViewModel, DeactivateAccountViewState> {
Expand Down