Downloaded files are corrupted when buffer length is > 1

Question

I'm trying to write a function which downloads a file at a specific URL. The function produces a corrupt file unless I make the buffer an array of size 1 (as it is in the code below).

The ternary statement above the buffer initialization (which I plan to use) along with hard-coded integer values other than 1 will manufacture a corrupted file.

Note: MAX_BUFFER_SIZE is a constant, defined as 8192 (2^13) in my code.

public static void downloadFile(String webPath, String localDir, String fileName) {
    try {
        File localFile;
        FileOutputStream writableLocalFile;
        InputStream stream;

        url = new URL(webPath);
        HttpURLConnection connection = (HttpURLConnection) url.openConnection();

        int size = connection.getContentLength(); //File size in bytes
        int read = 0; //Bytes read

        localFile = new File(localDir);

        //Ensure that directory exists, otherwise create it.
        if (!localFile.exists())
            localFile.mkdirs();

        //Ensure that file exists, otherwise create it.
        //Note that if we define the file path as we do below initially and call mkdirs() it will create a folder with the file name (I.e. test.exe). There may be a better alternative, revisit later.
        localFile = new File(localDir + fileName);
        if (!localFile.exists())
            localFile.createNewFile();

        writableLocalFile = new FileOutputStream(localFile);
        stream = connection.getInputStream();

        byte[] buffer;
        int remaining;
        while (read != size) {
            remaining = size - read; //Bytes still to be read
            //remaining > MAX_BUFFER_SIZE ? MAX_BUFFER_SIZE : remaining
            buffer = new byte[1]; //Adjust buffer size according to remaining data (to be read).

            read += stream.read(buffer); //Read buffer-size amount of bytes from the stream.
            writableLocalFile.write(buffer, 0, buffer.length); //Args: Bytes to read, offset, number of bytes
        }

        System.out.println("Read " + read + " bytes.");

        writableLocalFile.close();
        stream.close();
    } catch (Throwable t) {
        t.printStackTrace();
    }
}

The reason I've written it this way is so I may provide a real time progress bar to the user as they are downloading. I've removed it from the code to reduce clutter.

score 1 · Accepted Answer · answered Dec 28 '18 at 13:27

len = stream.read(buffer);
read += len;
writableLocalFile.write(buffer, 0, len);

You must not use buffer.length as the bytes read, you need to use the return value of the read call. Because it might return a short read and then your buffer contains junk (0 bytes or data from previous reads) after the read bytes.

And besides calculating the remaining and using dynamic buffers just go for 16k or something like that. The last read will be short, which is fine.

I cannot believe I've wasted 2 hours on this! Thank you very much for your help! — celoj, Dec 28 '18 at 13:36

score 1 · Answer 2 · answered Dec 28 '18 at 13:30

InputStream.read() may read number of bytes fewer than you requested. But you always append whole buffer to the file. You need to capture actual number of read bytes and append only those bytes to the file.

Additionally:

Watch for InputStream.read() to return -1 (EOF)
Server may return incorrect size. As such, the check read != size is dangerous. I would advise not to rely on the Content-Length HTTP field altogether. Instead, just keep reading from the input stream until you hit EOF.

Thanks for your notes! I'll be sure to update my code accordingly. — celoj, Dec 28 '18 at 13:37

Downloaded files are corrupted when buffer length is > 1

2 Answers2