Improve handling of NullPointerException in MMapDirectory's IndexInputs (check the "closed" condition)

[uschindler]

See the dev thread by @msokolov @ https://lists.apache.org/thread/qts8wvrjs54gkgz04pk4p93fg0wjbq3o

The handling of NPE is very special in ByteBufferIndexInput and also MemorySegmentIndexInput: To signal a closed input we set the buffers to NULL so any code trying to work on the inputs hits an NullPointerException. This is to avoid any null checks or isOpen checks everywhere in the code, which might be expensive, as the variable/field is not constant.

But it must on the other hand be avoided that the NPE gets visible outside of the IndexInputs, because it looks like a buggy null checks and may cause support issues. So we have to hide it by all means, as the NPE is not an error but a signal that the IndexInput was closed.

There are a few cases where a real NPE can still happen, e.g., when somebody accidentally passes a null array to one of the read methods. In that case the NPE is important and should be re-thrown unmodified for the visibility of the caller.

The workaround here to not add the NPE as a cause (and making it visible to call er outside code) is to do a safety check when the excapetion actually happens (so it does not slow down anything): If the NPE is catched, the code in this PR checks that the "closed" condition applies to current state of IndexInput (buffers are null or the byte buffer guard is invalided). If and only if the "closed" check is true, it throws AlreadyCosedException. In all other cases it rethrows the original NPE.

I also added a test which failed before my change to demonstrate that it works. We may possibly add this check also to BaseDirectoryTestCase,

The changes were applied to all MemorySegmentIndexInput variants in the MR-JAR and the original ByteBufferIndexInput (+ its guard).

[msokolov]

Thanks! This would have exposed the NPE I stumbled on, so +1 for that

It would also be nice if we could somehow find a way not to swallow every IllegalStateException, but this is already an improvement. um OK I see we are asserting that can only happen in one way. If that's the case, it seems fine, although a bit fragile to maintain?

[uschindler]

IllegalStateException cannot happen in that code, only in access to memory segments closed by other threads.

NPE was a special case as it may happen easier. IllegalStateException has a clear meaning and cannot happen unless you call a method that documents to throw it.

[uschindler]

If that's the case, it seems fine, although a bit fragile to maintain?

I argued during the long journey of Panama Foreign to have a specific subclass of IllegalStateException for the case of unmapping a MemorySegment from another thread. It is too late. You could look into exception message but that's even more fragile due to localization.

[uschindler]

If that's the case, it seems fine, although a bit fragile to maintain?

I argued during the long journey of Panama Foreign to have a specific subclass of IllegalStateException for the case of unmapping a MemorySegment from another thread. It is too late. You could look into exception message but that's even more fragile due to localization.

I think in contrast to the signalling NPE, the IllegalStateException is not hurting if it appears as cause of the AlreadyClosedException. The error message also tells you that the segment was closed by another thread, which might be helpful. I will think about it and possibly make a new PR that improves this for MemorySegmentIndexInput.

I can also figure out if there is a check for MemorySegments if they are unreachable. I think they added something in recent versions. It may not work in Java 19, but later ones can be improved.

Uwe

[uschindler]

You can call segment.scope().isAlive() to figure out if the scope is still alive. This works for Java 20+. The Java 19 version can't use this.

I will possibly create a new PR to handle this and also rethrow the IllegalStateException as done here for NPE.

[uschindler]

I improved the IllegalStateHandling in #12707 in the same way by confirming the state of the segment's scope (Java20+) / session (Java19).

@msokolov: Please have a quick look before I merge it.