Convert AccountService to suspend functions
#2354
Conversation
Signed-off-by: Dominic Fischer <dominicfischer7@gmail.com>
matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/common/CommonTestHelper.kt
Outdated
Show resolved
Hide resolved
| session.deactivateAccount(action.password, action.eraseAllData) | ||
| DeactivateAccountViewEvents.Done | ||
| } catch (failure: Exception) { | ||
| if (failure is CancellationException) throw failure |
There was a problem hiding this comment.
Why CancellationException is treated apart, and why throwing? Will it make the app crash?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I guess for most of the cases it will be ok yeah
|
Please also run to cleanup the code curl -sSLO https://github.com/pinterest/ktlint/releases/download/0.34.2/ktlint && chmod a+x ktlint
./ktlint --android --experimental -v |
Signed-off-by: Dominic Fischer <dominicfischer7@gmail.com>
Signed-off-by: Dominic Fischer <dominicfischer7@gmail.com>
There was a problem hiding this comment.
Thanks for the update.
I will fix the conflict and merge the PR (EDIT: there are no conflicts, this is on another PR).
We have to think about the other applications which uses the SDK, and which are maybe not ready to use coroutines. Maybe for them we will create a project which converts coroutines to MatrixCallback. But for Element and its forks, we think it's a great improvement of the SDK API :)
Signed-off-by: Dominic Fischer dominicfischer7@gmail.com
Pull Request Checklist
Would do a big PR but this will be easier to review.
Also each endpoint needs to be checked to see which dispatchers it should use so it's best to keep it small.