Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@
import com.google.appinventor.components.runtime.util.AnimationUtil;
import com.google.appinventor.components.runtime.util.BulkPermissionRequest;
import com.google.appinventor.components.runtime.util.ErrorMessages;
import com.google.appinventor.components.runtime.util.FileCache;
import com.google.appinventor.components.runtime.util.FileUtil;
import com.google.appinventor.components.runtime.util.FullScreenVideoUtil;
import com.google.appinventor.components.runtime.util.JsonUtil;
Expand Down Expand Up @@ -291,6 +292,8 @@ public class Form extends AppInventorCompatActivity
// FragmentActivity is added in future.
public static final int MAX_PERMISSION_NONCE = 100000;

public FileCache fileCache;
Copy link
Member

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.




public static class PercentStorageRecord {
Expand Down Expand Up @@ -331,6 +334,10 @@ public void onCreate(Bundle icicle) {
// Called when the activity is first created
super.onCreate(icicle);

if (fileCache == null) {
this.fileCache = new FileCache(new java.io.File(this.getCacheDir(), "file_cache"));
}

// This version is for production apps. See {@link ReplForm#onCreate} for the REPL version,
// which overrides this method.
Thread.setDefaultUncaughtExceptionHandler(new Thread.UncaughtExceptionHandler() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package com.google.appinventor.components.runtime.util;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add standard preamble here.


import java.io.File;
import java.util.HashMap;
import java.util.concurrent.CompletableFuture;
import java.util.logging.Level;
import java.util.logging.Logger;

public class FileCache {
public File cacheDir;
Copy link
Member

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.

private final HashMap<String, CompletableFuture<Void>> fileMap = new HashMap<>();
Copy link
Member

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.

private static final Logger LOG = Logger.getLogger(FileCache.class.getName());

/**
* Creates a new FileCache instance with the specified cache directory. If the directory doesn't
* exist, it will be created.
*
* @param cacheDir the directory to use for caching files
*/
public FileCache(File cacheDir) {
this.cacheDir = cacheDir;
if (!cacheDir.exists()) {
cacheDir.mkdirs();
Copy link
Member

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.

Copy link
Contributor Author

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?

}
}

/**
* Registers a file for download from the specified URL to the cache. If the file doesn't exist in
* the cache, it will be downloaded asynchronously.
*
* @param path the relative path within the cache directory where the file formshould be stored
* @param url the URL from which to download the file
* @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) {
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

final File file = new File(cacheDir, path);
if (!file.exists()) {
CompletableFuture<Void> future = CompletableFuture.runAsync(new Runnable() {
@Override
public void run() {
try {
FileUtil.downloadUrlToFile(url, file.getAbsolutePath());
fileMap.remove(path);
Copy link
Member

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.

} catch (Exception error) {
LOG.log(Level.SEVERE, "Exception downloading file to cache", error);
}
}
});
fileMap.put(path, future);
return future;
Copy link
Member

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.

}
return CompletableFuture.completedFuture(null);
}

/**
* Retrieves a file from the cache at the specified path. If the file is currently being
* downloaded, this method will wait for the download to complete.
*
* @param path the relative path of the file within the cache directory
* @return a CompletableFuture containing the File if it exists and is ready, or a failed future
* with an exception if the file doesn't exist or download failed
*/
public CompletableFuture<File> getFile(String path) {
File file = new File(cacheDir, path);
if (!file.exists()) {
return CompletableFuture.failedFuture(new Exception("File does not exist: " + path));
} else if (fileMap.containsKey(path)) {
try {
fileMap.get(path).get();
Copy link
Member

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.

} catch (Exception e) {
return CompletableFuture.failedFuture(e);
}
return CompletableFuture.completedFuture(file);
} else {
return CompletableFuture.completedFuture(file);
}
}

/**
* Recursively deletes a folder and all its contents. This is a helper method used by
* resetCache().
*
* @param folder the folder to delete
*/
private void deleteFolder(File folder) {
Copy link
Member

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.

if (folder.isDirectory()) {
for (File file : folder.listFiles()) {
deleteFolder(file);
}
}
folder.delete();
}

/**
* Resets the cache by deleting all cached files and clearing the internal file map. This will
* remove the entire cache directory and recreate it as an empty directory.
*/
public void resetCache() {
if (cacheDir.exists()) {
deleteFolder(cacheDir);
}
fileMap.clear();
}
Comment on lines 174 to 193
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should think about if this is really necessary. If extensions call this for example it may corrupt existing component instances that assume the file is already cached (NPE due to missing entry in the table).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty easy to corrupt the file cache by just initiating a fetch without wifi or cutting off wifi midway through a download. We need to be able to reset the cache because of how easily it can be corrupted. There is no other way to verify file integrity and automatically do it unless we refetch every time we have internet, which uses a lot of bandwidth for no reason, especially when the types of files being cached are large. We also can't rely on the ETag header because it is not mandatory.

}