-
-
Notifications
You must be signed in to change notification settings - Fork 23
fix: Close streams to be able to delete cache folder #70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping the patcher version would be needed in this PR but see the comment i left
@@ -54,6 +54,7 @@ object ApkUtils { | |||
ZFile.openReadWrite(apkFile, zFileOptions).use { targetApkZFile -> | |||
dexFiles.forEach { dexFile -> | |||
targetApkZFile.add(dexFile.name, dexFile.stream) | |||
dexFile.stream.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, however now you can't call this function twice. I remember functions should "always be callable multiple times"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it maybe possible to pass a handle to manually open and close a stream in patcher? Not a file handle though as we'd need to expose the "File" API when patcher should just contract with a generic handle instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my knowledge, a File
object is the lowest/generic way to construct a stream from a local file. Once a stream is closed, the data stream no longer exists and you can't re-open or close it at will.
This is pretty much why I advocated to pass around a file descriptor instead of a potentially closed stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most abstract is a Handle interface with a method to open a stream to be implemented by patcher. Not a File object
interface Handle {
fun inputStream()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already explained why File won't work. The fact that patcher is using a File under the hood is private and scoped to patchers internals. I may very much swap it with an in memory implementation. The second i expose the File API, it is part of the public API and I can't do that no more. Right now patcher provides a dexFile.stream directly, but instead it can provide a handle to open a stream.
e.g:
h = result.dexHandle
stream = openStream(h)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're the maintainer, so up to you. Feel free the close the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To pass a handle, patcher API needs to change to return an interface where you can call inputStream(). Patcher then can handle the implementation for it respectively for the internal implementation. For File ill open a file inputstream, for byte[] it'll wrap to a bytearray input stream. Any amount of streams can be opened and closed solving the issue in this comment. Could you PR that? Since the API changes, you may need to deprecate the old API
Added in conjunction with ReVanced/revanced-patcher#343
The parent
use()
block will call the finalizer of the input streams when it closes itself.ReVanced/revanced-cli#327