From 8f1b6992719422dfb94754e418521958beafe9da Mon Sep 17 00:00:00 2001 From: Marco Romano Date: Thu, 9 Nov 2023 15:15:11 +0100 Subject: [PATCH] Don't leak MediaFileHandle when downloading voice messages (#1748) Uses the new `MediaFile.persist()` [API](https://github.com/matrix-org/matrix-rust-sdk/pull/2789) to cache voice messages. This allows to close the `MediaFile` handle after use and keeping the file. Also disables rust sdk caching for voice messages as we'll use the app's cache dir for that purpose. Fixes: https://github.com/vector-im/element-meta/issues/2175 --- .../timeline/VoiceMessageMediaRepo.kt | 16 ++++++++-------- .../matrix/api/media/MatrixMediaLoader.kt | 8 +++++++- .../libraries/matrix/api/media/MediaFile.kt | 8 +++++++- .../libraries/matrix/impl/media/RustMediaFile.kt | 4 ++++ .../matrix/impl/media/RustMediaLoader.kt | 9 +++++++-- .../libraries/matrix/test/media/FakeMediaFile.kt | 5 +++++ .../matrix/test/media/FakeMediaLoader.kt | 7 ++++++- 7 files changed, 44 insertions(+), 13 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt index 98b43ae415..a36eccc1d2 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessageMediaRepo.kt @@ -24,7 +24,6 @@ import io.element.android.libraries.di.CacheDirectory import io.element.android.libraries.di.RoomScope import io.element.android.libraries.matrix.api.media.MatrixMediaLoader import io.element.android.libraries.matrix.api.media.MediaSource -import io.element.android.libraries.matrix.api.media.toFile import java.io.File /** @@ -90,14 +89,15 @@ class DefaultVoiceMessageMediaRepo @AssistedInject constructor( source = mediaSource, mimeType = mimeType, body = body, + useCache = false, ).mapCatching { - val dest = cachedFile.apply { parentFile?.mkdirs() } - // TODO By not closing the MediaFile we're leaking the rust file handle here. - // Not that big of a deal but better to avoid it someday. - if (it.toFile().renameTo(dest)) { - dest - } else { - error("Failed to move file to cache.") + it.use { mediaFile -> + val dest = cachedFile.apply { parentFile?.mkdirs() } + if (mediaFile.persist(dest.path)) { + dest + } else { + error("Failed to move file to cache.") + } } } } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MatrixMediaLoader.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MatrixMediaLoader.kt index 8dd5c625d1..288cfb3777 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MatrixMediaLoader.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MatrixMediaLoader.kt @@ -35,7 +35,13 @@ interface MatrixMediaLoader { * @param source to fetch the data for. * @param mimeType: optional mime type. * @param body: optional body which will be used to name the file. + * @param useCache: if true, the rust sdk will cache the media in its store. * @return a [Result] of [MediaFile] */ - suspend fun downloadMediaFile(source: MediaSource, mimeType: String?, body: String?): Result + suspend fun downloadMediaFile( + source: MediaSource, + mimeType: String?, + body: String?, + useCache: Boolean = true, + ): Result } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaFile.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaFile.kt index d4989dbffc..eb478aca80 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaFile.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/media/MediaFile.kt @@ -21,10 +21,16 @@ import java.io.File /** * A wrapper around a media file on the disk. - * When closed the file will be removed from the disk. + * When closed the file will be removed from the disk unless [persist] has been used. */ interface MediaFile : Closeable { fun path(): String + + /** + * Persists the temp file to the given path. The file will be moved to + * the given path and won't be deleted anymore when closing the handle. + */ + fun persist(path: String): Boolean } fun MediaFile.toFile(): File { diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaFile.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaFile.kt index 4b26b8c6c6..3274bfc123 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaFile.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaFile.kt @@ -25,6 +25,10 @@ class RustMediaFile(private val inner: MediaFileHandle) : MediaFile { return inner.path() } + override fun persist(path: String): Boolean { + return inner.persist(path) + } + override fun close() { inner.close() } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt index 50fb5dc6a5..12a273f03c 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/media/RustMediaLoader.kt @@ -71,7 +71,12 @@ class RustMediaLoader( } } - override suspend fun downloadMediaFile(source: MediaSource, mimeType: String?, body: String?): Result = + override suspend fun downloadMediaFile( + source: MediaSource, + mimeType: String?, + body: String?, + useCache: Boolean, + ): Result = withContext(mediaDispatcher) { runCatching { source.toRustMediaSource().use { mediaSource -> @@ -79,7 +84,7 @@ class RustMediaLoader( mediaSource = mediaSource, body = body, mimeType = mimeType ?: MimeTypes.OctetStream, - useCache = true, + useCache = useCache, tempDir = cacheDirectory.path, ) RustMediaFile(mediaFile) diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaFile.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaFile.kt index 275580d11e..71ca07ae30 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaFile.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaFile.kt @@ -17,11 +17,16 @@ package io.element.android.libraries.matrix.test.media import io.element.android.libraries.matrix.api.media.MediaFile +import java.io.File class FakeMediaFile(private val path: String) : MediaFile { override fun path(): String { return path } + override fun persist(path: String): Boolean { + return File(path()).renameTo(File(path)) + } + override fun close() = Unit } diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaLoader.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaLoader.kt index 8ceb878adf..3cf01f49bf 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaLoader.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/media/FakeMediaLoader.kt @@ -42,7 +42,12 @@ class FakeMediaLoader : MatrixMediaLoader { } } - override suspend fun downloadMediaFile(source: MediaSource, mimeType: String?, body: String?): Result = simulateLongTask { + override suspend fun downloadMediaFile( + source: MediaSource, + mimeType: String?, + body: String?, + useCache: Boolean, + ): Result = simulateLongTask { if (shouldFail) { Result.failure(RuntimeException()) } else {