[Android] Handles file creation failures when attempting to retry failed downloads #771
Conversation
rupinderjeet
force-pushed
the
fix/wrong-assumption-about-saving-files-in-public-storage
branch
from
b396370 to
ea8af83
Compare
| if (newFile.exists()) { | ||
| val deleted = newFile.delete() | ||
| if (!deleted) { | ||
| logError("Unable to delete existing file: ${newFile.absolutePath}") | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think it's expected to delete the old file to make place for the the newly downloaded file.
If new file creation fails, we wrongly assume that the user is creating a file in public storage but not setting
saveInPublicStoragetotrue.
Can we modify the error message to state not setting saveInPublicStorage is one of the possible causes, not the cause?
There was a problem hiding this comment.
I don't think it's expected to delete the old file to make place for the the newly downloaded file.
What should be done in this case? fileName passed in createFileInAppSpecificDir(String, String) already exists. Can we create a file with different (randomised index) name and return it? I think it will cause issues upstream from this function.
Can we modify the error message to state not setting saveInPublicStorage is one of the possible causes, not the cause?
Yes, I wrote it as a question to the user (in the commit):
"Unable to create new file: ${newFile.absolutePath}.
Are are trying to save file in public storage but not setting 'saveInPublicStorage' to 'true'?"
This can definitely be updated to state it differently.
There was a problem hiding this comment.
Yes, I wrote it as a question to the user (in the commit):
Yes, but the developer sees this log after the original file is already deleted.
Please also see this issue, I remember some agreement was reached in that issue what should we do in that case. But it's kinda abandoned, feel free to continue their work if you have time and will :)
|
I will create a new pull request. |
If new file creation fails, we wrongly assume that the user is creating a file in public storage but not setting
saveInPublicStoragetotrue. File creation can, also, fail if the file already exists. There might be other IO cases as well (low memory etc.) where file creation fails.Since we're creating a new file, I think it should be okay to overwrite any existing files with same file-path. If not, this PR fails to address the issue.
I have added some code to delete the existing file, and then, create the new file. I am not aware whether this issue exists on iOS side or not.
When I tried to retry some downloads, the android code will throw the null pointer exception since
createFileInAppSpecificDir(String, String)returns null in this case.