Got error "inappropriate format" when returning a file inside of a try block

Question

I am trying to create a password-protected zip file using zip4j that contains several files. This is the code I have so far

fun createZipFile(filename: String, vararg containers: Pair<String, ByteArray?>): ByteArray {
     val out = ByteArrayOutputStream()
     val zipFile = ZipOutputStream(out, passwordGenerator.generate());
     try {
         for ((name, file) in containers.toList()) {
             if (file != null) {
                 val parameters = createZipParameters(name)
                 zipFile.putNextEntry(parameters)
                 zipFile.write(file)
                 zipFile.closeEntry()
             }
         }
         // return here cause error
         return out.toByteArray()
     }
     finally {
         zipFile.close()
         out.close()
     }
 }

I received an "inappropriate format" error when I tried to open the file. However, when I made a small change and moved the return statement after the finally block, it worked fine

   // return out.toByteArray()
}
finally {
   zipFile.close()
   out.close()
}
return out.toByteArray()

Can you explain why this change made a difference and why the original code did not work?

score 4 · Accepted Answer · answered Apr 28 '23 at 18:12

4

The crucial thing you're missing is that you have heard an oversimplification that is incorrect and has now led you astray. What you think you know/heard is:

finally blocks run before the return statement.

But that is incorrect. The correct thought is:

finally blocks run before returning.

As in, first out.toByteArray() is evaluated, then the finally block runs (closing the zip stream, which causes it to write the zip header, because in zip files, those are at the end), then the evaluated-before-running-the-finally byte array is returned. Which doesn't have that header because it didn't yet when it was created. Whereas in your second snippet, the zip stream writes its header out prior to evaluating out.toByteArray().

For what its worth, try/finally is useless here - none of these things are system resources that demand that you safely close them. The code is a lot simpler to read if you just make it:

fun createZipFile(filename: String, vararg containers: Pair<String, ByteArray?>): ByteArray {
     val out = ByteArrayOutputStream()
     val zipFile = ZipOutputStream(out, passwordGenerator.generate());
     for ((name, file) in containers.toList()) {
         if (file != null) {
             val parameters = createZipParameters(name)
             zipFile.putNextEntry(parameters)
             zipFile.write(file)
             zipFile.closeEntry()
         }
     }
     zipFile.close()
     return out.toByteArray()
 }

A few notes on the above:

There is no need to out.close() - closing a wrapper stream also closing the thing it wraps.
If your linter complains, fix your linter, don't fix your code. A linter exists to make your code easier to read. If you write harder to read code to avoid it, you need to ask yourself some serious questions.

answered Apr 28 '23 at 18:12

rzwitserloot

85,357
5
51
72

I wonder what would happen if there's an error throw before reaching zipFile.close(). would this cause memory leak? – Patrick Apr 28 '23 at 18:18
@Patrick Not necessarily a "memory leak" _per se_, but it could lead to other issues. That's why you should always use [try-with-resources](https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html) in such cases (Java). The equivalent in Kotlin is [`use`](https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.io/use.html). However, neither are strictly needed in this case because you're writing to a `ByteArrayOutputStream`. – Slaw Apr 28 '23 at 19:17
Patrick: No, why would it? You just have some object that will eventually be garbage collected. strings don't have a `close()` method at all, for example. Neither `zipFile` nor that instance of `ByteArrayOutputStream` holds anything at all except memory. Ignore @Slaw 's comment. Making vague overtures to undefined "other issues"? It's meaningless. – rzwitserloot Apr 29 '23 at 01:43
@Patrick try-with is for e.g. `new FileOutputStream()`, or `socket.getInputStream()`, or `Class.myClass.getResourceAsStream()`, or `Files.newBufferedReader` or `jdbcDriverManager.getConnection()` - things that make an actual not-in-java 'handle' object (OS-level file handle, Database connection, that sort of thing). Java garbage collects its own stuff but can't garbage collect your DB or your OS file handles for you. Hence, the need to always close them, hence, `try (var x = new resource()) {}` syntax. Here there are no such objects at all, and therefore, no need. – rzwitserloot Apr 29 '23 at 01:46
I disagree that I made "vague overtures", or more specifically, I disagree that they're meaningless. I simply didn't go into details. Your second comment addresses the other issues I mentioned. They're important to keep in mind, as you've explained. And I did state that these other issues don't need to be worried about in this case, _specifically because the OP is writing to memory_. Though perhaps that could have been clearer. Anyway, my comment was addressing the OP's question in a more general sense rather than specifically `zipFile.close()` in the example code. – Slaw Apr 29 '23 at 04:16

Got error "inappropriate format" when returning a file inside of a try block

1 Answers1