Skip to content

Ensure getRootSpaceSummaries() is not called on the main thread. #5835

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
bmarty merged 9 commits into from
Apr 26, 2022
Merged

Ensure getRootSpaceSummaries() is not called on the main thread. #5835

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cd06ba6
Ensure `getRootSpaceSummaries()` is not called on the main thread.
bmarty Apr 25, 2022
fb73628
Rename val
bmarty Apr 25, 2022
d2eca73
Apply suggestion
bmarty Apr 25, 2022
abdfd9d
Format
bmarty Apr 25, 2022
c542619
We do not need to observe the user here
bmarty Apr 25, 2022
b2f9d67
Version hotfix 1.4.13
bmarty Apr 26, 2022
da656ac
Changelog
bmarty Apr 26, 2022
ebcab18
towncrier
bmarty Apr 26, 2022
6b5822e
fastlane (same content than for 1.4.12)
bmarty Apr 26, 2022
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
4 changes: 3 additions & 1 deletion ...k-android/src/androidTest/java/org/matrix/android/sdk/session/space/SpaceHierarchyTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,9 @@ class SpaceHierarchyTest : InstrumentedTest {
// + C
// + c1, c2

val rootSpaces = session.spaceService().getRootSpaceSummaries()
val rootSpaces = commonTestHelper.runBlockingTest {
session.spaceService().getRootSpaceSummaries()
}

assertEquals("Unexpected number of root spaces ${rootSpaces.map { it.name }}", 2, rootSpaces.size)

Expand Down
5 changes: 4 additions & 1 deletion matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/space/SpaceService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,8 @@ interface SpaceService {

suspend fun removeSpaceParent(childRoomId: String, parentSpaceId: String)

fun getRootSpaceSummaries(): List<RoomSummary>
/**
* Get the root spaces, i.e. all the spaces which do not have a parent space.
*/
suspend fun getRootSpaceSummaries(): List<RoomSummary>
}
11 changes: 8 additions & 3 deletions ...ndroid/src/main/java/org/matrix/android/sdk/internal/session/space/DefaultSpaceService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package org.matrix.android.sdk.internal.session.space

import android.net.Uri
import androidx.lifecycle.LiveData
import kotlinx.coroutines.withContext
import org.matrix.android.sdk.api.MatrixCoroutineDispatchers
import org.matrix.android.sdk.api.query.QueryStringValue
import org.matrix.android.sdk.api.session.events.model.Event
import org.matrix.android.sdk.api.session.events.model.EventType
Expand Down Expand Up @@ -64,7 +66,8 @@ internal class DefaultSpaceService @Inject constructor(
private val stateEventDataSource: StateEventDataSource,
private val peekSpaceTask: PeekSpaceTask,
private val resolveSpaceInfoTask: ResolveSpaceInfoTask,
private val leaveRoomTask: LeaveRoomTask
private val leaveRoomTask: LeaveRoomTask,
private val coroutineDispatchers: MatrixCoroutineDispatchers,
) : SpaceService {

override suspend fun createSpace(params: CreateSpaceParams): String {
Expand Down Expand Up @@ -105,8 +108,10 @@ internal class DefaultSpaceService @Inject constructor(
return roomSummaryDataSource.getSpaceSummaries(spaceSummaryQueryParams, sortOrder)
}

override fun getRootSpaceSummaries(): List<RoomSummary> {
return roomSummaryDataSource.getRootSpaceSummaries()
override suspend fun getRootSpaceSummaries(): List<RoomSummary> {
return withContext(coroutineDispatchers.io) {

@ouchadam ouchadam Apr 26, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

not for this PR as it would introduce lots of changes, but worth highlighting in case we want guard against other cases

from my understanding the general approach of coroutines scoping is to define the context switching at the lowest possible level, which in this case would be

monarchy.fetchAllMappedSync

we already have a similar wrapper for awaitTransaction

this would also force non coroutine based usages to rely on runBlocking, which would have the benefit of make finding the usages a bit easier

@bmarty bmarty Apr 26, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree, good point!

@ganfra ganfra Apr 26, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually we might want to dispatch querying db to a background thread, to avoid blocking the dispatcher, as Retrofit does for request (and probably Room)

roomSummaryDataSource.getRootSpaceSummaries()
}
}

override suspend fun peekSpace(spaceId: String): SpacePeekResult {
Expand Down
26 changes: 9 additions & 17 deletions vector/src/main/java/im/vector/app/features/spaces/SpaceListViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.sample
import kotlinx.coroutines.launch
Expand Down Expand Up @@ -212,7 +211,8 @@ class SpaceListViewModel @AssistedInject constructor(@Assisted initialState: Spa
}
session.coroutineScope.launch {
orderCommands.forEach {
session.getRoom(it.spaceId)?.updateAccountData(RoomAccountDataTypes.EVENT_TYPE_SPACE_ORDER,
session.getRoom(it.spaceId)?.updateAccountData(
RoomAccountDataTypes.EVENT_TYPE_SPACE_ORDER,
SpaceOrderContent(order = it.order).toContent()
)
}
Expand Down Expand Up @@ -278,31 +278,23 @@ class SpaceListViewModel @AssistedInject constructor(@Assisted initialState: Spa
displayName = QueryStringValue.IsNotEmpty
}

val flowSession = session.flow()

combine(
flowSession
.liveUser(session.myUserId)
.map {
it.getOrNull()
},
flowSession
session.flow()
.liveSpaceSummaries(params),
session
.accountDataService()
session.accountDataService()
.getLiveRoomAccountDataEvents(setOf(RoomAccountDataTypes.EVENT_TYPE_SPACE_ORDER))
.asFlow()
) { _, communityGroups, _ ->
Comment on lines +284 to -295

@ouchadam ouchadam Apr 26, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to double check, were the other flows being used as some sort of predicate? / Do we need .getLiveRoomAccountDataEvents(...).asFlow()

it's a bit strange to see us submit flows to combine and then ignore their result 🤔

@bmarty bmarty Apr 26, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's true that we ignore the result, but we want to be notified when the account data EVENT_TYPE_SPACE_ORDER is updated.

communityGroups
) { spaces, _ ->
spaces
}
.execute { async ->
val rootSpaces = session.spaceService().getRootSpaceSummaries()
val orders = rootSpaces.map {
val rootSpaces = async.invoke().orEmpty().filter { it.flattenParentIds.isEmpty() }

@bmarty bmarty Apr 25, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On my side this is equivalent, but I do not have any spaces with child space. I will create some

@bmarty bmarty Apr 25, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, it works as expected

val orders = rootSpaces.associate {

@ouchadam ouchadam Apr 26, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💯

it.roomId to session.getRoom(it.roomId)
?.getAccountDataEvent(RoomAccountDataTypes.EVENT_TYPE_SPACE_ORDER)
?.content.toModel<SpaceOrderContent>()
?.safeOrder()
}.toMap()
}
copy(
asyncSpaces = async,
rootSpacesOrdered = rootSpaces.sortedWith(TopLevelSpaceComparator(orders)),
Expand Down