Compress up to 24mb files into .zip using ZipOutputStream in Android

Question

I am trying to zip directories from one area (sdCard/someFolder) into a second directory (sdCard/Download), until the .zip file size becomes 5mb. Then, I want to create a new .zip file, fill that new one up to 5mb, etc.

Currently, my code compresses files into .zip directories successfully, but one of the .zip directories always becomes corrupt. I see this when my for loop exits the first Files[] of 22 objects and starts the next directory with Files[] of 4 objects. I believe I am losing some cleanup of old OutputStreams. The out.putNextEntry() becomes null after the second attempt of the for loop. Any help will suffice.

private static void addDirToArchive(ZipOutputStream out, FileOutputStream destinationDir, File sdCardMNDLogs)
{
    File[] listOfFiles = sdCardMNDLogs.listFiles();

    BufferedInputStream origin = null;

    Log.i(TAG3, "Reading directory: " + sdCardMNDLogs.getName());

    try{

    byte[] buffer = new byte[BUFFER];
    for(int i = 0; i < listOfFiles.length; i++)
    {
        if(listOfFiles[i].isDirectory())
        {
            addDirToArchive(out, destinationDir, listOfFiles[i]);
            continue;
        }
        try 
        {
            FileInputStream fis = new FileInputStream(listOfFiles[i]);
            origin = new BufferedInputStream(fis,BUFFER);
            ZipEntry ze = new ZipEntry(listOfFiles[i].getName());

            if(currentZipFileSize >= EMAIL_SIZE)
            {
                out.close();
                Log.d(emailTAG, "Creating new zipfile: /Download/MND/nwdLogs_" + i);
                out = new ZipOutputStream(new FileOutputStream(new File(sdCard.getAbsolutePath() + "/Download/MND/nwdLogs_ " + i + ".zip")));
                currentZipFileSize = 0;
            }
            out.putNextEntry(ze);
            int length;
            Log.i(TAG3, "Adding file: " + listOfFiles[i].getName());
            while((length = origin.read(buffer, 0, BUFFER)) != -1)
            {
                out.write(buffer, 0, length);
            }
            out.closeEntry();
            origin.close();
            currentZipFileSize = currentZipFileSize + ze.getCompressedSize();
        }
        catch(IOException ioe)
        {
            Log.e(TAG3, "IOException: " + ioe);
        }
    }
    }
    finally
    {
        try {
            out.close();
    } catch (IOException e) 
    {
        e.printStackTrace();
    }
}

}

FileOutputStream destinationDir = new FileOutputStream(sdCard.getAbsolutePath() + "/Download/Dir/nwdLogs.zip");
ZipOutputStream out = new ZipOutputStream(destinationDir);

currentZipFileSize = 0;
addDirToArchive(out, destinationDir, dirName);
out.close();
destinationDir.close();

Answer 1

I suspect that the problem is that you aren't calling out.close() before opening the next ZIP file. My understanding is that ZIP's index is only written when the ZIP is closed, so if you neglect to close the index will be missing: hence corruption.

Also, note that you don't need to close both fis and origin. Just close origin ... and it closes fis.

---

UPDATE - While you have fixed the original close bug, there are more:

1. You have added a finally block to close out. That is wrong. You don't want addDirToArchive to close out. That's the likely cause of your exceptions.

2. There are a couple of problems that happen after you have done this:

   if (currentZipFileSize >= EMAIL_SIZE)
       {
           out.close();
           out = new ZipOutputStream(new FileOutputStream(...));
           currentZipFileSize = 0;
       }
   

   Since out is a local parameter, the caller does not see the change you make. Therefore:

   • when you call out.close() in the caller, you could be closing the original ZIP (already closed) ... not the current one

   • if you called addDirToArchive(out, destinationDir, dirName) multiple times, in subsequent calls you could be passing a closed ZIP file.

3. Your exception handling is misguided (IMO). If there is an I/O error writing a file to the ZIP, you do NOT want log a message and keep going. You want to bail out. Either crash the app entirely, or stop doing what you are doing. In this case, you "stream is closed" is clearly a bug in your code, and your exception handling is effectively telling the app to ignore it.

Some advice:

• If you are splitting the responsibility for opening and closing resources across multiple methods you need to be VERY careful about what code has responsibility for closing what. You need to understand what you are doing.

• Blindly applying (so called) "solutions" (like the finally stuff) ... 'cos someone says "XXX is best practice" or "always do XXX"... is going to get you into trouble. You need to 1) understand what the "solution" does, and 2) think about whether the solution actually does what you need.