Mitigate a deadlock when loading room timelines (#6674)
This may be happening because we were not destroying focused event timelines used for the media viewer/gallery when necessary, and having several of those back paginating *may* have caused a deadlock in the event cache.
This commit is contained in:
parent
24e68f6f76
commit
3bb7ab301f
9 changed files with 28 additions and 19 deletions
|
|
@ -17,6 +17,7 @@ import io.element.android.libraries.matrix.api.room.BaseRoom
|
||||||
import io.element.android.libraries.matrix.api.timeline.Timeline
|
import io.element.android.libraries.matrix.api.timeline.Timeline
|
||||||
import io.element.android.libraries.matrix.api.timeline.item.event.toEventOrTransactionId
|
import io.element.android.libraries.matrix.api.timeline.item.event.toEventOrTransactionId
|
||||||
import io.element.android.libraries.mediaviewer.impl.model.GroupedMediaItems
|
import io.element.android.libraries.mediaviewer.impl.model.GroupedMediaItems
|
||||||
|
import kotlinx.coroutines.CoroutineScope
|
||||||
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
||||||
import kotlinx.coroutines.flow.Flow
|
import kotlinx.coroutines.flow.Flow
|
||||||
import kotlinx.coroutines.flow.MutableSharedFlow
|
import kotlinx.coroutines.flow.MutableSharedFlow
|
||||||
|
|
@ -27,10 +28,11 @@ import kotlinx.coroutines.flow.launchIn
|
||||||
import kotlinx.coroutines.flow.map
|
import kotlinx.coroutines.flow.map
|
||||||
import kotlinx.coroutines.flow.onCompletion
|
import kotlinx.coroutines.flow.onCompletion
|
||||||
import kotlinx.coroutines.flow.onEach
|
import kotlinx.coroutines.flow.onEach
|
||||||
|
import timber.log.Timber
|
||||||
import java.util.concurrent.atomic.AtomicBoolean
|
import java.util.concurrent.atomic.AtomicBoolean
|
||||||
|
|
||||||
interface MediaGalleryDataSource {
|
interface MediaGalleryDataSource {
|
||||||
fun start()
|
fun start(coroutineScope: CoroutineScope)
|
||||||
fun groupedMediaItemsFlow(): Flow<AsyncData<GroupedMediaItems>>
|
fun groupedMediaItemsFlow(): Flow<AsyncData<GroupedMediaItems>>
|
||||||
fun getLastData(): AsyncData<GroupedMediaItems>
|
fun getLastData(): AsyncData<GroupedMediaItems>
|
||||||
suspend fun loadMore(direction: Timeline.PaginationDirection)
|
suspend fun loadMore(direction: Timeline.PaginationDirection)
|
||||||
|
|
@ -58,7 +60,7 @@ class TimelineMediaGalleryDataSource(
|
||||||
private val isStarted = AtomicBoolean(false)
|
private val isStarted = AtomicBoolean(false)
|
||||||
|
|
||||||
@OptIn(ExperimentalCoroutinesApi::class)
|
@OptIn(ExperimentalCoroutinesApi::class)
|
||||||
override fun start() {
|
override fun start(coroutineScope: CoroutineScope) {
|
||||||
if (!isStarted.compareAndSet(false, true)) {
|
if (!isStarted.compareAndSet(false, true)) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
@ -96,9 +98,12 @@ class TimelineMediaGalleryDataSource(
|
||||||
groupedMediaItemsFlow.emit(AsyncData.Success(groupedMediaItems))
|
groupedMediaItemsFlow.emit(AsyncData.Success(groupedMediaItems))
|
||||||
}
|
}
|
||||||
.onCompletion {
|
.onCompletion {
|
||||||
timeline?.close()
|
timeline?.let {
|
||||||
|
Timber.d("Timeline media gallery data source flow completed for room ${room.roomId}, closing timeline")
|
||||||
|
it.close()
|
||||||
|
}
|
||||||
}
|
}
|
||||||
.launchIn(room.roomCoroutineScope)
|
.launchIn(coroutineScope)
|
||||||
}
|
}
|
||||||
|
|
||||||
override suspend fun loadMore(direction: Timeline.PaginationDirection) {
|
override suspend fun loadMore(direction: Timeline.PaginationDirection) {
|
||||||
|
|
|
||||||
|
|
@ -78,7 +78,7 @@ class MediaGalleryPresenter(
|
||||||
.collectAsState(AsyncData.Uninitialized)
|
.collectAsState(AsyncData.Uninitialized)
|
||||||
|
|
||||||
LaunchedEffect(Unit) {
|
LaunchedEffect(Unit) {
|
||||||
mediaGalleryDataSource.start()
|
mediaGalleryDataSource.start(this)
|
||||||
}
|
}
|
||||||
|
|
||||||
val permissions by room.permissionsAsState(MediaPermissions.DEFAULT) { perms ->
|
val permissions by room.permissionsAsState(MediaPermissions.DEFAULT) { perms ->
|
||||||
|
|
|
||||||
|
|
@ -35,6 +35,7 @@ import kotlinx.collections.immutable.ImmutableList
|
||||||
import kotlinx.collections.immutable.persistentListOf
|
import kotlinx.collections.immutable.persistentListOf
|
||||||
import kotlinx.collections.immutable.toImmutableList
|
import kotlinx.collections.immutable.toImmutableList
|
||||||
import kotlinx.coroutines.CoroutineDispatcher
|
import kotlinx.coroutines.CoroutineDispatcher
|
||||||
|
import kotlinx.coroutines.CoroutineScope
|
||||||
import kotlinx.coroutines.flow.Flow
|
import kotlinx.coroutines.flow.Flow
|
||||||
import kotlinx.coroutines.flow.map
|
import kotlinx.coroutines.flow.map
|
||||||
import kotlinx.coroutines.withContext
|
import kotlinx.coroutines.withContext
|
||||||
|
|
@ -62,11 +63,12 @@ class MediaViewerDataSource(
|
||||||
private val localMediaStates: MutableMap<String, MutableState<AsyncData<LocalMedia>>> =
|
private val localMediaStates: MutableMap<String, MutableState<AsyncData<LocalMedia>>> =
|
||||||
mutableMapOf()
|
mutableMapOf()
|
||||||
|
|
||||||
fun setup() {
|
fun setup(coroutineScope: CoroutineScope) {
|
||||||
galleryDataSource.start()
|
galleryDataSource.start(coroutineScope)
|
||||||
}
|
}
|
||||||
|
|
||||||
fun dispose() {
|
fun dispose() {
|
||||||
|
Timber.d("Disposing MediaViewerDataSource, closing ${mediaFiles.size} media files")
|
||||||
mediaFiles.forEach { it.close() }
|
mediaFiles.forEach { it.close() }
|
||||||
mediaFiles.clear()
|
mediaFiles.clear()
|
||||||
localMediaStates.clear()
|
localMediaStates.clear()
|
||||||
|
|
|
||||||
|
|
@ -88,7 +88,7 @@ class MediaViewerPresenter(
|
||||||
var mediaBottomSheetState by remember { mutableStateOf<MediaBottomSheetState>(MediaBottomSheetState.Hidden) }
|
var mediaBottomSheetState by remember { mutableStateOf<MediaBottomSheetState>(MediaBottomSheetState.Hidden) }
|
||||||
|
|
||||||
DisposableEffect(Unit) {
|
DisposableEffect(Unit) {
|
||||||
dataSource.setup()
|
dataSource.setup(coroutineScope)
|
||||||
onDispose {
|
onDispose {
|
||||||
dataSource.dispose()
|
dataSource.dispose()
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -20,12 +20,13 @@ import io.element.android.libraries.mediaviewer.impl.datasource.MediaGalleryData
|
||||||
import io.element.android.libraries.mediaviewer.impl.model.GroupedMediaItems
|
import io.element.android.libraries.mediaviewer.impl.model.GroupedMediaItems
|
||||||
import io.element.android.libraries.mediaviewer.impl.model.MediaItem
|
import io.element.android.libraries.mediaviewer.impl.model.MediaItem
|
||||||
import kotlinx.collections.immutable.persistentListOf
|
import kotlinx.collections.immutable.persistentListOf
|
||||||
|
import kotlinx.coroutines.CoroutineScope
|
||||||
import kotlinx.coroutines.flow.flowOf
|
import kotlinx.coroutines.flow.flowOf
|
||||||
|
|
||||||
class SingleMediaGalleryDataSource(
|
class SingleMediaGalleryDataSource(
|
||||||
private val data: GroupedMediaItems,
|
private val data: GroupedMediaItems,
|
||||||
) : MediaGalleryDataSource {
|
) : MediaGalleryDataSource {
|
||||||
override fun start() = Unit
|
override fun start(coroutineScope: CoroutineScope) = Unit
|
||||||
override fun groupedMediaItemsFlow() = flowOf(AsyncData.Success(data))
|
override fun groupedMediaItemsFlow() = flowOf(AsyncData.Success(data))
|
||||||
override fun getLastData(): AsyncData<GroupedMediaItems> = AsyncData.Success(data)
|
override fun getLastData(): AsyncData<GroupedMediaItems> = AsyncData.Success(data)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -13,6 +13,7 @@ import io.element.android.libraries.matrix.api.core.EventId
|
||||||
import io.element.android.libraries.matrix.api.timeline.Timeline
|
import io.element.android.libraries.matrix.api.timeline.Timeline
|
||||||
import io.element.android.libraries.mediaviewer.impl.model.GroupedMediaItems
|
import io.element.android.libraries.mediaviewer.impl.model.GroupedMediaItems
|
||||||
import io.element.android.tests.testutils.lambda.lambdaError
|
import io.element.android.tests.testutils.lambda.lambdaError
|
||||||
|
import kotlinx.coroutines.CoroutineScope
|
||||||
import kotlinx.coroutines.flow.Flow
|
import kotlinx.coroutines.flow.Flow
|
||||||
import kotlinx.coroutines.flow.MutableSharedFlow
|
import kotlinx.coroutines.flow.MutableSharedFlow
|
||||||
|
|
||||||
|
|
@ -21,7 +22,7 @@ class FakeMediaGalleryDataSource(
|
||||||
private val loadMoreLambda: (Timeline.PaginationDirection) -> Unit = { lambdaError() },
|
private val loadMoreLambda: (Timeline.PaginationDirection) -> Unit = { lambdaError() },
|
||||||
private val deleteItemLambda: (EventId) -> Unit = { lambdaError() },
|
private val deleteItemLambda: (EventId) -> Unit = { lambdaError() },
|
||||||
) : MediaGalleryDataSource {
|
) : MediaGalleryDataSource {
|
||||||
override fun start() = startLambda()
|
override fun start(coroutineScope: CoroutineScope) = startLambda()
|
||||||
|
|
||||||
private val groupedMediaItemsFlow = MutableSharedFlow<AsyncData<GroupedMediaItems>>(
|
private val groupedMediaItemsFlow = MutableSharedFlow<AsyncData<GroupedMediaItems>>(
|
||||||
replay = 1
|
replay = 1
|
||||||
|
|
|
||||||
|
|
@ -80,7 +80,7 @@ class TimelineMediaGalleryDataSourceTest {
|
||||||
roomCoroutineScope = backgroundScope,
|
roomCoroutineScope = backgroundScope,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
sut.start()
|
sut.start(backgroundScope)
|
||||||
assertThat(sut.getLastData()).isEqualTo(AsyncData.Uninitialized)
|
assertThat(sut.getLastData()).isEqualTo(AsyncData.Uninitialized)
|
||||||
sut.groupedMediaItemsFlow().test {
|
sut.groupedMediaItemsFlow().test {
|
||||||
assertThat(awaitItem().isLoading()).isTrue()
|
assertThat(awaitItem().isLoading()).isTrue()
|
||||||
|
|
@ -95,7 +95,7 @@ class TimelineMediaGalleryDataSourceTest {
|
||||||
)
|
)
|
||||||
assertThat(sut.getLastData().isSuccess()).isTrue()
|
assertThat(sut.getLastData().isSuccess()).isTrue()
|
||||||
// Also test that starting again should have no effect
|
// Also test that starting again should have no effect
|
||||||
sut.start()
|
sut.start(backgroundScope)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Ensure that the timeline has been closed on flow completion
|
// Ensure that the timeline has been closed on flow completion
|
||||||
|
|
@ -117,7 +117,7 @@ class TimelineMediaGalleryDataSourceTest {
|
||||||
roomCoroutineScope = backgroundScope,
|
roomCoroutineScope = backgroundScope,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
sut.start()
|
sut.start(backgroundScope)
|
||||||
sut.groupedMediaItemsFlow().test {
|
sut.groupedMediaItemsFlow().test {
|
||||||
skipItems(2)
|
skipItems(2)
|
||||||
sut.loadMore(Timeline.PaginationDirection.BACKWARDS)
|
sut.loadMore(Timeline.PaginationDirection.BACKWARDS)
|
||||||
|
|
@ -140,7 +140,7 @@ class TimelineMediaGalleryDataSourceTest {
|
||||||
roomCoroutineScope = backgroundScope,
|
roomCoroutineScope = backgroundScope,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
sut.start()
|
sut.start(backgroundScope)
|
||||||
sut.groupedMediaItemsFlow().test {
|
sut.groupedMediaItemsFlow().test {
|
||||||
skipItems(2)
|
skipItems(2)
|
||||||
sut.deleteItem(AN_EVENT_ID)
|
sut.deleteItem(AN_EVENT_ID)
|
||||||
|
|
@ -159,7 +159,7 @@ class TimelineMediaGalleryDataSourceTest {
|
||||||
roomCoroutineScope = backgroundScope,
|
roomCoroutineScope = backgroundScope,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
sut.start()
|
sut.start(backgroundScope)
|
||||||
sut.groupedMediaItemsFlow().test {
|
sut.groupedMediaItemsFlow().test {
|
||||||
assertThat(awaitItem().isLoading()).isTrue()
|
assertThat(awaitItem().isLoading()).isTrue()
|
||||||
assertThat(sut.getLastData().isLoading()).isTrue()
|
assertThat(sut.getLastData().isLoading()).isTrue()
|
||||||
|
|
@ -181,7 +181,7 @@ class TimelineMediaGalleryDataSourceTest {
|
||||||
roomCoroutineScope = backgroundScope,
|
roomCoroutineScope = backgroundScope,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
sut.start()
|
sut.start(backgroundScope)
|
||||||
sut.groupedMediaItemsFlow().test {
|
sut.groupedMediaItemsFlow().test {
|
||||||
assertThat(awaitItem().isLoading()).isTrue()
|
assertThat(awaitItem().isLoading()).isTrue()
|
||||||
assertThat(sut.getLastData().isLoading()).isTrue()
|
assertThat(sut.getLastData().isLoading()).isTrue()
|
||||||
|
|
|
||||||
|
|
@ -50,7 +50,7 @@ class MediaViewerDataSourceTest {
|
||||||
val sut = createMediaViewerDataSource(
|
val sut = createMediaViewerDataSource(
|
||||||
galleryDataSource = galleryDataSource,
|
galleryDataSource = galleryDataSource,
|
||||||
)
|
)
|
||||||
sut.setup()
|
sut.setup(backgroundScope)
|
||||||
startLambda.assertions().isCalledOnce()
|
startLambda.assertions().isCalledOnce()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -37,9 +37,9 @@ class SingleMediaGalleryDataSourceTest {
|
||||||
val warmUpRule = WarmUpRule()
|
val warmUpRule = WarmUpRule()
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `function start is no op`() {
|
fun `function start is no op`() = runTest {
|
||||||
val sut = SingleMediaGalleryDataSource(aGroupedMediaItems())
|
val sut = SingleMediaGalleryDataSource(aGroupedMediaItems())
|
||||||
sut.start()
|
sut.start(backgroundScope)
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue