fix: Replace dex input streams with file references #343
Conversation
| public fun <init> (Ljava/lang/String;Ljava/lang/Object;Ljava/util/Map;Ljava/lang/String;Ljava/lang/String;ZLkotlin/reflect/KType;Lkotlin/jvm/functions/Function2;)V | ||
| public synthetic fun <init> (Ljava/lang/String;Ljava/lang/Object;Ljava/util/Map;Ljava/lang/String;Ljava/lang/String;ZLkotlin/reflect/KType;Lkotlin/jvm/functions/Function2;ILkotlin/jvm/internal/DefaultConstructorMarker;)V | ||
| public fun <init> (Ljava/lang/String;Ljava/lang/Object;Ljava/util/Map;Ljava/lang/String;ZLkotlin/reflect/KType;Lkotlin/jvm/functions/Function2;)V | ||
| public synthetic fun <init> (Ljava/lang/String;Ljava/lang/Object;Ljava/util/Map;Ljava/lang/String;ZLkotlin/reflect/KType;Lkotlin/jvm/functions/Function2;ILkotlin/jvm/internal/DefaultConstructorMarker;)V |
There was a problem hiding this comment.
Not sure where these changes came from. Maybe previous merge that didn't run apiDump?
| public final class app/revanced/patcher/PatcherResult$PatchedDexFile { | ||
| public final fun getFile ()Ljava/io/File; | ||
| public final fun getName ()Ljava/lang/String; | ||
| public final fun getStream ()Ljava/io/InputStream; |
There was a problem hiding this comment.
This breaks API, please add this back with a deprecated annotation
| DexIO.DEFAULT_MAX_DEX_POOL_SIZE, | ||
| ) { _, entryName, _ -> logger.info { "Compiled $entryName" } } | ||
| }.listFiles(FileFilter { it.isFile })!!.map { | ||
| PatcherResult.PatchedDexFile(it.name, it.inputStream()) |
There was a problem hiding this comment.
The input stream can be simply closed here via a use block. No need to convert to a file.
There was a problem hiding this comment.
Just adding a use block here will leave scope and close the stream before PatcherResult.applyTo() is called.
This is why I advocate passing around a file reference instead of an open stream; it's much easier to reason about.
There was a problem hiding this comment.
The fact that Patcher is dumping the dex to a file is not something that any other library should take advantage of. It is merely done for caching purposes and no other purpose. In the most basic form patcher has no "requirements" for or does not require anyone to work with files. A stream is all that's needed. If a consuming library needs to convert it to a file it's free to, if one doesn't then there is no reason to require working with files in the first place. If the stream needs to be open, then the use block should not be used here and whatever consumes the stream should simply close the stream.
oSumAtrIX
left a comment
There was a problem hiding this comment.
I'll close the PR for now as there is no requirements of a file over a stream. Closing the stream should work the same as closing the file. Thanks!
This should fix the issue where the CLI cannot purge the temporary patch files because there's still file handles from hanging input streams.
I originally was going to just make
PatcherResultimplementCloseable, but I think it makes more sense to let Revanced library open and close the streams instead of passing them around.ReVanced/revanced-cli#327