On Wed, 10 Dec 2025 10:41:34 GMT, Maurizio Cimadamore <[email protected]> wrote:
>> Also, nulling out "sharedResources" during close() for better GC cleanup is >> doable, but not completely trivial, since it adds "post-closure" failure >> modes that didn't exist before. >> I do think that *somewhere* we should be detaching the index for garbage >> collection, but maybe all its users are sufficiently well scoped that it's >> unlikely to matter. > > In general, I prefer to err on the side of correctness (as you did here). > Given all allocation of JRTIndex go through the factory (and the shared file > system resources), if we detect something suspicious it likely means there's > an issue somewhere. Now, we could argue if this should be a crash or not -- > but if it's not then we'll likely never know about it. > > Note -- javac has a class to do some assertion checks -- see > com.sun.tools.javac.util.Assert. > > So this could also be rewritten as: > > > Assert.check(sharedResources.release(this), "Already closed"); I don't see this as an assertion because I could easily write a test which causes it to happen. To me, assertions are for code invariants that "should never happen" due to the immediate surrounding code and logic. This can be triggered by a badly behaved caller, which (due to the public nature of this class) could be anyone. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1761#discussion_r2622742076
