improve already downloaded contribution file check

[ryantm]

When downloading a file, it's possible that a corrupted archive could be
bigger, smaller, or the same size as a non-corrupted archive, so it is
not good to rely on the archive file size. Instead, the checksum can be
used to verify if the already downloaded file is viable for reuse.

[cmaglie]

With this patch you remove the chance to resume downloads.

it's possible that a corrupted archive could be bigger, smaller, or the same size as a non-corrupted archive

Not exactly, from the index.json we know size and checksum in advance, so we have three possibilities:

• if the downloaded file size is less than expected the file is certainly corrupted there is no reason to spend CPU time to calculate the checksum. But it's perfectly possible that the download may have been interrupted, in particular for large archives: in this case the best thing to do is to try to resume the download using the existing file as a partial download.
• If the downloaded size matches the expected size, the file integrity must be checked with CRC. If the CRC fails the file is certainly corrupted and should be downloaded again.
• If the downloaded size is more than expected then the file is certainly corrupted and should be downloaded again: no need to check CRC in this case.

[ryantm]

@cmaglie Thank you for reviewing my PR. I agree that we can get a bit of performance by checking the size, so I will update it to do that.

I do not understand how my changes remove the chance to resume downloads, if that chance previously existed. I didn't realize it had the feature of resuming downloads though, so that makes the name alreadyDownloadedFileViable wrong, but alreadyDownloadedFileViable should still return false if the file size is smaller than the contribution size.

Does the code currently allow for resuming partially completed downloads? How does the download resuming work?

[cmaglie]

Hi @ryantm,

yeah, sorry, indeed your PR doesn't break the resume I was misleaded by the fact that alreadyDownloadedFileViable always tests the checksum.

The download(...) method is the one that resume the download if outputFile already exists:
https://github.com/arduino/Arduino/blob/master/arduino-core/src/cc/arduino/utils/network/FileDownloader.java#L153

Now going back to your PR it seems to do:

1. check if the downloaded file is available:

• looks if the file exists
• checks CRC

2. if the file is not available download it

3. check the CRC

so, in case the file is already downloaded the CRC is checked twice. I guess that the alreadyDownloadedFileViable should check only if the file exists and it's size to avoid checking the checksum twice.

Also I'll rename alreadyDownloadedFileViable to alreadyDownloadedFileAvailable (I don't know if it's a typo).

How does it sounds?

[ryantm]

@cmaglie Please take a look at my latest commit 7ebecc2. I believe I addressed your concerns and improvement suggestions.

[cmaglie]

Honestly I'm missing the purpose of this PR: what is the problem are you trying to solve exactly?

[ffissore]

Closing for lack of feedback