Use streams to encode and decode the JSON backups #139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I can confirm this fixes the issue. Tested exporting on a Pixel 5 and importing on a Pixel 4 and all MMS and SMS were available. Generated archive was ~120MB. It's slow but it works. Please merge. |
When importing/exporting large files we can run out of memory, in which case we should display a toast to the user (as we do for other exceptions). Previously, the OutOfMemoryError was not caught because it inherits from Error, not from Exception. (We have to manually call .toString() because Context.showErrorToast() from Commons takes an Exception or a String, not Throwable.)
If ActivityResultContracts.CreateDocument launches the native Files app (com.android.documentsui), then it will create the file before the Uri is returned to us. So if we don't complete the export (due to an exception or because there are no messages) we should delete the file, otherwise there will be an empty/incomplete file left behind.
This significantly reduces memory usage. (We buffer the OutputStream because kotlinx.serialization flushes its internal buffers excessively[1]. We also buffer the InputStream for consistency, even though kotlinx.serialization uses adequate buffering; there is no performance impact because BufferedInputStream cascades harmlessly[2].) The functions encodeToStream() and decodeFromStream() are marked as experimental, but this is a pretty fundamental use case so surely it will continue to be supported in the future (maybe with minor changes). Fixes FossifyOrg#6. [1] https://github.com/Kotlin/kotlinx.serialization/blob/v1.6.3/formats/json/jvmMain/src/kotlinx/serialization/json/internal/JvmJsonStreams.kt#L46 [2] https://github.com/openjdk/jdk/blob/jdk-23%2B14/src/java.base/share/classes/java/io/BufferedInputStream.java#L339
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is it?
Description of the changes in your PR
Use streams to encode and decode the JSON backups instead of storing the file contents in memory.
This significantly reduces memory usage when importing/exporting backups.
Report OutOfMemoryError when importing and exporting messages. (Less likely to happen after this PR, but still possible.)
Delete exported file on error. (Fixes an existing issue that is somewhat related this PR.)
Implementation notes:
I initially tried to delete using
contentResolver.delete(uri, null, null), but that failed with "UnsupportedOperationException: Delete not supported" so I switched to DocumentsContract.deleteDocument().But when using the Fossify File Manager (FFM) as the file picker,
deleting doesn't seem to do anything in both cases.Correction: With FFM, contentResolver.delete() works but DocumentsContract.deleteDocument() doesn't do anything.
I think it's okay to just use DocumentsContract.deleteDocument(), so that it works well with the native file picker, and accept that if using FFM then the files won't be cleaned up on error. I don't think it's a big deal because I can't even choose FFM as the file picker on Android 14, and because FFM currently allows the user to pick an existing filename so it's probably not a good idea to delete when FFM is used. Also, it's possible that DocumentsContract.deleteDocument() not doing anything is a bug in FFM; if that's the case then that bug should be fixed instead of adding a workaround to Messages.
Before/After Screenshots/Screen Record
N/A
Fixes the following issue(s)
Other apps
There are several other apps that have a similar JSON import/export feature. Should I port this fix to them?
Future work
(Copied from #6 (comment))
The version in this PR still loads all the messages into RAM, so it can still run out of memory if the backup is too large.
It's possible to reduce the memory usage to virtually nothing, but it would take more work. The idea is to read the messages one at a time or in chunks, and then encode them and write them to the backup individually. It's not that bad -- we just need to manually write a "[" at the start, then write the messages (encoded individually and manually separated by ","), then write a "]" at the end.
Doing something equivalent for import should also be possible, using decodeToSequence.
@gusr has pointed out that the SMS Import / Export app (which is also GPL-3 and Kotlin) works better and suggested using their code. Personally I'm not sure if that's practical, but they have some good ideas that we could learn from. The core import/export code is here (snapshot).
Acknowledgement