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
This commit is contained in:
@@ -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.")
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<MediaFile>
|
||||
suspend fun downloadMediaFile(
|
||||
source: MediaSource,
|
||||
mimeType: String?,
|
||||
body: String?,
|
||||
useCache: Boolean = true,
|
||||
): Result<MediaFile>
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
@@ -71,7 +71,12 @@ class RustMediaLoader(
|
||||
}
|
||||
}
|
||||
|
||||
override suspend fun downloadMediaFile(source: MediaSource, mimeType: String?, body: String?): Result<MediaFile> =
|
||||
override suspend fun downloadMediaFile(
|
||||
source: MediaSource,
|
||||
mimeType: String?,
|
||||
body: String?,
|
||||
useCache: Boolean,
|
||||
): Result<MediaFile> =
|
||||
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)
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -42,7 +42,12 @@ class FakeMediaLoader : MatrixMediaLoader {
|
||||
}
|
||||
}
|
||||
|
||||
override suspend fun downloadMediaFile(source: MediaSource, mimeType: String?, body: String?): Result<MediaFile> = simulateLongTask {
|
||||
override suspend fun downloadMediaFile(
|
||||
source: MediaSource,
|
||||
mimeType: String?,
|
||||
body: String?,
|
||||
useCache: Boolean,
|
||||
): Result<MediaFile> = simulateLongTask {
|
||||
if (shouldFail) {
|
||||
Result.failure(RuntimeException())
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user