Close the JarFile when JarAnalyzer's constructor fails

[kratos0718]

Fixes #139

Problem

JarAnalyzer's constructor opens the JarFile, then lists and sorts its entries, and only afterwards reads the manifest:

this.jarFile = new JarFile(file);

List<JarEntry> entries = Collections.list(jarFile.entries());
entries.sort(Comparator.comparing(ZipEntry::getName));

Manifest manifest;
try {
    manifest = jarFile.getManifest();
} catch (IOException e) {
    closeQuietly();          // only this path cleans up
    throw e;
}

Only the manifest read is guarded. If the listing or the sort throws, the constructor exits with the JarFile still open — and because construction failed, the caller has no instance on which to call closeQuietly(), so the handle leaks. The class javadoc already promises the file "will be closed if this occurs".

Comparator.comparing(ZipEntry::getName) is one way in: it throws NullPointerException on an entry with a null name, which a malformed archive can carry.

Fix

Wrap everything between opening the file and assigning jarData, and release the handle on any IOException or RuntimeException before rethrowing. That covers the sort, the entry listing, and anything else added to this region later, rather than enumerating individual failure modes.

It also folds in the manifest cleanup that was already there, so there is one exit path instead of two — net effect is +11/-9 in a single method.

Note on #148

This is adjacent to your #148, which removes the ZipException re-wrap in the block immediately above. The two are semantically independent — that one changes how the open fails, this one changes what happens after a successful open — so they compose cleanly. Happy to rebase on top of it if it lands first, or to fold both into one change if you'd prefer.

Testing

mvn test — 78 tests across the module, all passing.

I did not add a test for the leak itself. Reproducing it needs an entry with a null name, which standard tooling won't produce, and asserting the descriptor was released needs the JarFile to be injectable — the class constructs it internally. On POSIX a "can the file be deleted afterwards" assertion passes whether or not the handle leaked, so it would be a test that only means something on Windows. I'd rather not add a misleading one, but if you want a specific approach here I'm happy to write it.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

[elharo]

Sorting of list is done by name --> Sort list by name

[elharo]

don't catch raw RuntimeException

[elharo]

can this throw? why is it in the try block?

[kratos0718]

All three addressed, and the middle one changed the shape of the fix for the better.

Catching RuntimeException was me reaching for the wrong tool. What the constructor actually needs is a release-on-failure guarantee, not an exception handler, so it now uses a constructed flag with finally and catches nothing at all. That also covers Error paths the old version silently missed.

On the entries list and the sort being inside the try: they are still in it, but the block is no longer a catch, so the question changes. Nothing in those two lines throws anything worth handling. They sit inside the finally scope only because the JarFile is already open by then, and anything that goes wrong between opening it and assigning jarData leaks the handle, since the constructor never returns and the caller gets no reference to close.

Comment reworded. mvn test is 78 passing.

[elharo]

Your try block isn't big enough. It should cover mist of the constructor to make finally work