IOException happens during writing to jar

[Ilya-Skiba]

Rolled back try-with resourses block, replaced with simple write #1881

[jpstotz]

Are you sure that without try-with-resource and without flush the manifest data are always fully written to destination? In my understanding there is a chance that manifest data is only written to the buffer inside BufferedOutputStream but never (completely) flushed to destination.

[Ilya-Skiba]

Yes, I'm sure. This flush happens during closeEntry invocation. Current implementation causes BufferedOutputStream to close, which forces ZipOutputStream to close. As a result, an exception occurs

[jpstotz]

How should the BufferedOutputStream be flushed in closeEntry? The reference to BufferedOutputStream is not known to closeEntry also destination does not know anything about the BufferedOutputStream instance, so how should BufferedOutputStream be flushed?

[Ilya-Skiba]

I've added a test for this case. In general, closing manifest entry forces all input buffers to flush. This why it works correctly

[StevenArzt]

I have to side with @jpstotz here. Calling closeEntry on a ZipOutputStream handles the re-positioning and the CRC calculation, but doesn't close any streams. The JavaDoc does not say that it closes streams and the code doesn't do it.

In my opinion, the call to closeEntry needs to be moved inside the try block. You flush, you call closeEntry, then the try block ends and the stream gets closed. With this approach, we don't get dangling streams and closeEntry should be happy.

[Ilya-Skiba]

According to closable, try-with-resources will close underlying ZipOutputStream, that is not correct for this method- ZipOutputStream might be used by other methods also. What do you think about removing BufferedOutputStream? I don't think there would be any issues with performance during small manifest file saving

[StevenArzt]

You're right, closing the BufferedOutputStream closes the inner stream as well.

The addManifest method should only be called by printZip and should only be called once there in line 317. In the stack trace in issue #1881, the line number is different, which confuses me slightly, but let's ignore that for now.

The printZip method owns the ZipOutputStream and the call to addManifest in line 317 is the last operation that does anything with the ZIP file. So no matter whether we close the inner stream in addManifest or not, noone should need that inner stream afterwards anyway. In fact, when printZip returns, it closes the ZipOutputStream anyway in its own try block (which I had overlooked in my previous post, so we don't get a dangling stream, even though closeEntry has nothing to do with it).

So whether we close the stream in adManifest or not, it shouldn't matter. However, this makes me wonder why changing addManifest has any effect on this exception, i.e., whether we're discussing something that is irrelevant to the problem at hand. If the entire ZIP file is closed after the call to addManifest, how can we later get an exception in addManifest? How can the control flow ever return there with the same ZipOutputStream? The ZipOutputStream should never be recycled anyway?

[Ilya-Skiba]

My code was based on the principle, that a stream should not be closed in a method, that did not open it. I've updated the code and returned try-with-resources

[StevenArzt]

That totally makes sense. I am now just confused why the exception from the issue report happens at all.

[Ilya-Skiba]

destination.closeEntry() failed before, because was trying to write entry on closed stream. You can rollback my changes in DexPrinter and run testWritingToJar test to reproduce