On Tue, 25 Nov 2025 15:30:51 GMT, David Beaumont <[email protected]> wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/file/JRTIndex.java line
>> 317:
>>
>>> 315: public void close() throws IOException {
>>> 316: // Release is atomic and succeeds at most once.
>>> 317: if (!sharedResources.release(this)) {
>>
>> This exception may be contentious. Please speak up if you don't like it.
>> It's here to track any accidental double-closing of the index, and did catch
>> one case already.
>> Since this is an internal class it feels like we should be able to track and
>> eliminate any double close events, but it is a runtime exception in complex
>> code, so I'm happy to remove it or maybe replace it with logging of some
>> kind.
>> Please speak up if you have opinions.
>
> 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");
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1761#discussion_r2606126565
