Add model file clean-up on initial load for all models found in share… #2353
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (a02ff14e) is created by Prow via merging commits: 843b963 f8e5bd1. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (a02ff14e) is created by Prow via merging commits: 843b963 f8e5bd1. |
...wnloader/src/main/java/com/google/firebase/ml/modeldownloader/internal/ModelFileManager.java
Outdated
Show resolved
Hide resolved
...wnloader/src/main/java/com/google/firebase/ml/modeldownloader/internal/ModelFileManager.java
Outdated
Show resolved
Hide resolved
| @@ -404,7 +409,12 @@ public File loadNewlyDownloadedModelFile(CustomModel model) { | |||
| new CustomModel( | |||
| model.getName(), model.getModelHash(), model.getSize(), 0, newModelFile.getPath())); | |||
|
|
|||
| // todo(annzimmer) Cleans up the old files if it is the initial creation. | |||
| try { | |||
There was a problem hiding this comment.
Since this is a clean-up task, it could probably be ran as a separate background task, WDYT?
There was a problem hiding this comment.
I wasn't sure if I should account for potential (and unlikely) weird threading issues if an app downloads two models almost back to back... Not even sure this would fully address that issue. Happy to change if you think it's the appropriate solution.
There was a problem hiding this comment.
Could you give an example of the situation? My mental model of the situation is as follows:
- ml downloads the latest model into a temp location
- once it's downloaded, it gets copied over to the definitive location.
- this code only checks for models in that definitive location
At which point the two downloads would collide?
Also, reading through it more carefully I noticed this: by calling listFiles() you get a snapshot of the files available at time X, then you iterate over it one by one, deleting the old models. Since the list of files is static, but the actual files are being deleted during iteration, could it be the case that you try to delete a file that has already being deleted earlier in the loop. Is this an issue?
There was a problem hiding this comment.
The listFiles snapshot is actually good. At the top level, only sub-directories are listed (which is the model names) and in the deeper level, all but the current model file are deleted for that model name.
I was concerned about...
Model A is getting downloaded and as the first one to hit clean up starts the deletion process.
Meanwhile, Model B (already downloaded) has a update and starts downloading.
During cleanup triggered by model A, we start the deletion of files associated with model B. We get the current file name (local file) and start deleting, at exactly the same time the update for model B finishes and copies the new file for B into the directory and updated the local file path... so we delete the new file and are now in a bad state.
This is extremely unlikely but I can probably fix if I do based on file # instead of and not passed in local file name. Let me see how hard that will be.
There was a problem hiding this comment.
Okay - check this - it should fix the updated while deleting issue.
I'm made the clean up into a task, is this the correct way to run it in the background?
There was a problem hiding this comment.
Thanks for the clarification, for a moment I though that all models were in the same directory (no subdirectories).
Sure thing, that's an approach. Let me know how it goes. Another possibility is to get the list of files and then check the name of the latest one, and then go deleting them by name. That way, if the file is added afterwards, it's not deleted.
There was a problem hiding this comment.
Actually this was updated and should work - the file name is the index... that way if a new one is added it's always a higher index. so new files won't be deleted if the sharedPreferences are now slightly behind the temporary file being moved to the correct spot.
…d preferences.