-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
File Cache API #3553
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: ucr
Are you sure you want to change the base?
File Cache API #3553
Conversation
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
@AppInventorWorkerBee ok to test |
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.
There are a number of potential thread safety issues we need to consider in this code, and as written would probably require a min SDK of 24, which isn't really on the table at the moment.
@@ -0,0 +1,105 @@ | |||
package com.google.appinventor.components.runtime.util; |
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.
Add standard preamble here.
|
||
public class FileCache { | ||
public File cacheDir; | ||
private final HashMap<String, CompletableFuture<Void>> fileMap = new HashMap<>(); |
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.
CompletableFuture
was added to Android with SDK 24. It would provide more compatibility to use Future
, which has been in the Android SDK since the beginning. Apps on older SDKs will not link correctly and code calling through this class will likely crash the app.
import java.util.logging.Logger; | ||
|
||
public class FileCache { | ||
public File cacheDir; |
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 don't know why this needs to be public. If it needs to be accessed from somewhere, make a getter function instead.
CompletableFuture<Void> future = CompletableFuture.runAsync(new Runnable() { | ||
@Override | ||
public void run() { | ||
try { | ||
FileUtil.downloadUrlToFile(url, file.getAbsolutePath()); | ||
fileMap.remove(path); | ||
} catch (Exception error) { | ||
LOG.log(Level.SEVERE, "Exception downloading file to cache", error); | ||
} | ||
} | ||
}); | ||
fileMap.put(path, future); | ||
return future; |
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.
Consider using our standard AsynchUtil class, especially if we remove the dependency on CompletableFuture.
* | ||
* @param folder the folder to delete | ||
*/ | ||
private void deleteFolder(File folder) { |
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.
Note that FileUtil has a removeDirectory
function that could be used instead.
public FileCache(File cacheDir) { | ||
this.cacheDir = cacheDir; | ||
if (!cacheDir.exists()) { | ||
cacheDir.mkdirs(); |
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.
Depending on the value of cacheDir
, there is the possibility this returns false
. If so, you probably want to throw an exception of some kind, probably an IOException
.
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.
Which specific value do I need to check to throw an IOException?
* @return a CompletableFuture that completes when the download is finished, or immediately if the | ||
* file already exists | ||
*/ | ||
public CompletableFuture<Void> registerFile(final String path, final String url) { |
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 should take a look at the AssetFetcher
and how it handles caching of HTTP resources, respecting the ETag header.
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.
Also, what happens if two different extensions use the same path
for different files, e.g., index.html
? My current reading is that the last one to finish downloading "wins" by overwriting the other file and any future calls to getFile will return the last file written.
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.
For the bottom comment, registration would just default to the current download task and not overwrite (the registerFile won't even attempt another download if one is currently in progress). With regards to handling the same path for different files, however, we should leave that up to the users of the file cache, since I don't think there's a really good way to ensure file uniqueness and map the correct files.
public void run() { | ||
try { | ||
FileUtil.downloadUrlToFile(url, file.getAbsolutePath()); | ||
fileMap.remove(path); |
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.
Note there is a potential race condition here where in theory the device could put the main thread to sleep in order to start the async call, and that finishes before the main thread resumes and adds the entry to the fileMap. You should synchronize access on fileMap to prevent this from happening.
return CompletableFuture.failedFuture(new Exception("File does not exist: " + path)); | ||
} else if (fileMap.containsKey(path)) { | ||
try { | ||
fileMap.get(path).get(); |
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.
See my note above about synchronizing access. It could happen that the main thread reaches the if statement above and branches. Then, before this line is executed it is put to sleep because network access completes on the other thread. That causes path
to be removed from the map and the future completes. Now, when this thread awakes, fileMap.get(path)
will return null and calling get() will raise NPE. This will cause a failed future to be returned on line 72 but in fact the operation was successful.
// FragmentActivity is added in future. | ||
public static final int MAX_PERMISSION_NONCE = 100000; | ||
|
||
public FileCache fileCache; |
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.
Make this private final
and provide a getter function.
General items:
ant tests
passes on my machineIf your code changes how something works on the device (i.e., it affects the companion):
ucr
ucr
as the baseFurther, if you've changed the blocks language or another user-facing designer/blocks API (added a SimpleProperty, etc.):
For all other changes:
master
master
as the baseWhat does this PR accomplish?
Description
This PR introduces a File Cache API that lets components and extensions download, cache, and reuse large external assets (e.g., ML model files, language libraries) directly on the device. Removing the need to bundle them in the APK and allowing for offline use after the initial fetch.
Highlights
com.google.appinventor.components.runtime.util.FileCache
FileCache
instance is attached to theForm
(form.fileCache
) to avoid collisions with other cache uses.CompletableFuture<Void>
so callers can await completion or launch multiple downloads in parallel without blocking the UI.CompletableFuture<File>
that resolves once the asset is present, transparently waiting if the file is still downloading.FileUtil.downloadUrlToFile
; avoids I/O on the main thread and keeps downloads independent.Typical usage
This mechanism allows for features such as Teachable LLM and similar tools that rely on assets too large to ship inside an extension, while preserving users’ ability to run the app offline after the initial download.
Resolves #3533