Ensure getRootSpaceSummaries() is not called on the main thread.
#5835
Conversation
| } | ||
| .execute { async -> | ||
| val rootSpaces = session.spaceService().getRootSpaceSummaries() | ||
| val rootSpaces = async.invoke().orEmpty().filter { it.flattenParentIds.isEmpty() } |
There was a problem hiding this comment.
On my side this is equivalent, but I do not have any spaces with child space. I will create some
| .getLiveRoomAccountDataEvents(setOf(RoomAccountDataTypes.EVENT_TYPE_SPACE_ORDER)) | ||
| .asFlow() | ||
| ) { _, communityGroups, _ -> | ||
| communityGroups |
There was a problem hiding this comment.
should rename this to spaceList
There was a problem hiding this comment.
I agree, but I wanted to limit the number of changes for the hotfix
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
LGTM, it's fixing freeze issues on my side
| override fun getRootSpaceSummaries(): List<RoomSummary> { | ||
| return roomSummaryDataSource.getRootSpaceSummaries() | ||
| override suspend fun getRootSpaceSummaries(): List<RoomSummary> { | ||
| return withContext(coroutineDispatchers.io) { |
There was a problem hiding this comment.
👍
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
There was a problem hiding this comment.
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)
| session.accountDataService() | ||
| .getLiveRoomAccountDataEvents(setOf(RoomAccountDataTypes.EVENT_TYPE_SPACE_ORDER)) | ||
| .asFlow() | ||
| ) { _, communityGroups, _ -> |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
It's true that we ignore the result, but we want to be notified when the account data EVENT_TYPE_SPACE_ORDER is updated.
| val rootSpaces = session.spaceService().getRootSpaceSummaries() | ||
| val orders = rootSpaces.map { | ||
| val rootSpaces = async.invoke().orEmpty().filter { it.flattenParentIds.isEmpty() } | ||
| val orders = rootSpaces.associate { |
ouchadam
left a comment
ouchadam
left a comment
There was a problem hiding this comment.
from the fix perspective LGTM! 💯 should this PR point to main ?
|
The PR could have pointed to |
Draft for a hotfix