Convert StateService to suspend functions #2500

aqulu · 2020-12-08T10:50:00Z

Pull Request Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Pull request is based on the develop branch
Pull request updates CHANGES.md
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off

Summary

Converted StateService to suspendable functions [SDK] Migrate Services API from MatrixCallback mode to coroutines #2449
RxRoom launches suspendable functions using kotlinx-coroutines-rx2 rxCompletable
- rxCompletable internally launches the suspendable function on GlobalScope. The job will be cancelled when the resulting Completable is, which is similar to completableBuilder's behavior in matrix-sdk-android-rx
WidgetPostAPIHandler uses GlobalScope.launch to run suspendable functions
- Attention: Directly using GlobalScope is discouraged.
- As WidgetPostAPIHandler is not tied to any lifecycle. Using a new CoroutineScope instance does not solve the issue, as running jobs would not be cancelled by lifecycle events.
  - WidgetPostAPIHandler seems to be used only by WidgetViewModel so it could potentially be tied to it's lifecycle to avoid using GlobalScope.
- Prior to these changes, the function calls were not constrained either, thus using GlobalScope seems appropriate.
  - StateService internally used TaskExecutor's coroutine scope. All jobs in this scope will be cancelled when CacheService.clearCache is invoked. As far as i understand this is a side-effect of cache clearing rather than it's primary goal.

Signed-off-by: aqulu <[email protected]>

bmarty

Thanks for your PR.
I got just one question, and I will be more comfortable if @ganfra can have a look on the change regarding the Widget part.
Also the CI is not happy, could you fix that please?
Thanks!

bmarty · 2020-12-08T11:39:00Z

...roid/src/main/java/org/matrix/android/sdk/internal/session/room/state/DefaultStateService.kt

-                .executeBy(taskExecutor)
+            body: JsonDict
+    ) {
+        withContext(coroutineDispatchers.main) {


Why changing the context here?

Sorry, didn't read executeBy(taskExecutor) internals properly.
TaskExecutor.execute ran this on the callback's thread (by transforming it with toDispatcher() internally). I'll remove the context change.

bmarty · 2020-12-08T11:39:38Z

...roid/src/main/java/org/matrix/android/sdk/internal/session/room/state/DefaultStateService.kt

-    override fun updateJoinRule(joinRules: RoomJoinRules?, guestAccess: GuestAccess?, callback: MatrixCallback<Unit>): Cancelable {
-        return taskExecutor.executorScope.launchToCallback(coroutineDispatchers.main, callback) {
+    override suspend fun updateJoinRule(joinRules: RoomJoinRules?, guestAccess: GuestAccess?) {
+        withContext(coroutineDispatchers.main) {


same question here

This was launched this in the coroutineDispatchers.main context by taskExecutor.executorScope.launchToCallback(coroutineDispatchers.main, callback) before.
I'm happy to remove the explicit withContext though.

bmarty · 2020-12-08T11:40:10Z

...roid/src/main/java/org/matrix/android/sdk/internal/session/room/state/DefaultStateService.kt

-    override fun updateAvatar(avatarUri: Uri, fileName: String, callback: MatrixCallback<Unit>): Cancelable {
-        return taskExecutor.executorScope.launchToCallback(coroutineDispatchers.main, callback) {
+    override suspend fun updateAvatar(avatarUri: Uri, fileName: String) {
+        withContext(coroutineDispatchers.main) {


same as https://github.com/vector-im/element-android/pull/2500/files/c7efd1feb99179585e865affa866d680a96fefe2#r538291540

aqulu · 2020-12-08T12:06:52Z

Thanks for the review!

Also the CI is not happy, could you fix that please?

I'll fix the CI errors and get rid of the context-changes you pointed out.

Signed-off-by: aqulu <[email protected]>

aqulu · 2020-12-08T14:28:26Z

CI should be fixed now. Looks like buildkite's "Compile and run Unit tests" job hung up - executing the command on my local machine completes successfully.

Dominaezzz · 2020-12-08T23:08:54Z

vector/src/main/java/im/vector/app/features/widgets/WidgetPostAPIHandler.kt

+                        stateKey = null,
+                        body = params
+                )
+                widgetPostAPIMediator.sendSuccess(eventData)


Should this be done inside the try block? If it throws, sendError will also be called, which isn't strictly the previous behaviour.
(Same for below)

Thanks for pointing this out! Fixed in ed822be

Dominaezzz · 2020-12-08T23:27:59Z

I've been quite busy lately and haven't been able to contribute to this, but I wasn't expecting anybody else to do these.
Might be worth "reserving" each service in the tracking issue so no one duplicates work because these can be annoying enough to untangle and maintain previous behaviour, don't want my (or anyone else's) work to be made redundant 😭.

Also, feel free to tag me to review these.

Signed-off-by: aqulu <[email protected]>

aqulu · 2020-12-08T23:50:22Z

I've been quite busy lately and haven't been able to contribute to this, but I wasn't expecting anybody else to do these.

@Dominaezzz Sorry for interfering with these, that wasn't my intention 🙇
I'll hold off from the other converting tasks, as it might be more consistent if done by the same person.

Dominaezzz · 2020-12-08T23:57:28Z

Oh no! 😨 Don't stop on my behalf! It would be amazing to have these done sooner than later aha.
I don't currently have a lot of time, so it would be awesome if you continued.
Consistency can be solved with reviews but I don't think there's much to worry about there anyway.

Looking forward to your next PR! 🎉 😀

bmarty

LGTM now, thanks for the update!
It seems that we have touch the StateService. Would you mind to rebase your change on develop, or merge develop to your branch?

bmarty · 2020-12-11T15:53:37Z

Actually I think I can do it with Github UI

Convert StateService to suspend functions

c7efd1f

Signed-off-by: aqulu <[email protected]>

bmarty requested a review from ganfra December 8, 2020 11:37

bmarty reviewed Dec 8, 2020

View reviewed changes

aqulu added 4 commits December 8, 2020 22:25

Fix failing test compilation

416f57b

Signed-off-by: aqulu <[email protected]>

Remove unused imports

c889dea

Signed-off-by: aqulu <[email protected]>

Remove coroutine context change for sendStateEvent

19d421d

Signed-off-by: aqulu <[email protected]>

Remove explicit coroutine context changes

40b9f03

Signed-off-by: aqulu <[email protected]>

Dominaezzz reviewed Dec 8, 2020

View reviewed changes

Fix try-catch behavior of sendStateEvent actions

ed822be

Signed-off-by: aqulu <[email protected]>

bmarty approved these changes Dec 11, 2020

View reviewed changes

Merge branch 'develop' into feature/state_service_coroutines

5b74eb3

bmarty merged commit 3eba43f into element-hq:develop Dec 11, 2020

aqulu deleted the feature/state_service_coroutines branch December 12, 2020 00:13

bmarty mentioned this pull request Mar 31, 2021

[SDK] Migrate Services API from MatrixCallback mode to coroutines #2449

Open

53 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert StateService to suspend functions #2500

Convert StateService to suspend functions #2500

aqulu commented Dec 8, 2020 •

edited

Loading

bmarty left a comment

bmarty Dec 8, 2020

aqulu Dec 8, 2020

bmarty Dec 8, 2020

aqulu Dec 8, 2020

bmarty Dec 8, 2020

aqulu Dec 8, 2020

aqulu commented Dec 8, 2020

aqulu commented Dec 8, 2020

Dominaezzz Dec 8, 2020

aqulu Dec 8, 2020

Dominaezzz commented Dec 8, 2020

aqulu commented Dec 8, 2020

Dominaezzz commented Dec 8, 2020

bmarty left a comment

bmarty commented Dec 11, 2020

Convert StateService to suspend functions #2500

Convert StateService to suspend functions #2500

Conversation

aqulu commented Dec 8, 2020 • edited Loading

Pull Request Checklist

Summary

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqulu commented Dec 8, 2020

aqulu commented Dec 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dominaezzz commented Dec 8, 2020

aqulu commented Dec 8, 2020

Dominaezzz commented Dec 8, 2020

bmarty left a comment

Choose a reason for hiding this comment

bmarty commented Dec 11, 2020

aqulu commented Dec 8, 2020 •

edited

Loading