Hi,
please don't add the NPE here as cause (except for debugging). The NPE
is only catched to NOT add extra checks in the highly performance
sensitive code. Actually the NPE is catched to detect the case where the
bytebuffer was already unset to trigger the already closed. The code
uses setting the buffers to NULL to signal cause, but it does NOT add a
NULL check everywhere. This allows Hotspot to compile this code without
any bounds checks and signal the AlreadyClosedException only when a NPE
happens. Adding the NPE as cause would bring confusion to end user, as
we only want to tell that IndexInput was closed, but the NPE should be
kept behind scenes as it would be a support nightmare ("your code has no
good null checks, it is broken"). The NPE is a signal here, not the cause.
I think the issue you have seen may be cause by passing a NULL parameter
to one of the methods like a float array to readFloats(). This is not
detected (P.S.: this also affects MemorySegmentIndexInput).
I can looks at the code to figure out how to better detect the NPE when
parameters of methods are NULL, but no way to add the cause here. I
would say: If you have to debug, do it temporarily or ose another
dircetory impl.
Uwe
Am 20.10.2023 um 21:20 schrieb Chris Hostetter:
FWIW: The choice to ignore the original exception goes back to here...
https://issues.apache.org/jira/browse/LUCENE-3588
...circa 2011, where it was focused on catching NPE and throwing
AlreadyClosedException instead, w/o any particular discussion as to why to
throw away the original NPE.
If i had to guess it's simply because at that time AlreadyClosedException
didn't support wrapping any other Throwable. That wasn't added until
LUCENE-5958 (circa 2014) which was focused on making sure "tragic" errors
kept a record of what caused the tragedy, and then include that as the
'cause' of the AlreadyClosedExceptions throw by 'ensureOpen()'
There didn't seem to be any discussion at that time about reviewing other
code that might be throwing AlreadyClosedException from a 'catch' block
that could also be updated to include the cause.
I'd say open a PR to review & update all code that results in
AlreadyClosedException originating from a catch block?
: Date: Tue, 17 Oct 2023 11:24:03 -0400
: From: Michael Sokolov <msoko...@gmail.com>
: Reply-To: dev@lucene.apache.org
: To: Lucene Dev <dev@lucene.apache.org>
: Subject: ByteBufferIndexInput.alreadyClosed creates an exception that doesn't
: track its cause
:
: I was messing around with something that was resulting in
: AlreadyClosedException being thrown and I noticed that we weren't
: tracking the exception that caused it. I found this in
: ByteBufferIndexInput:
:
: // the unused parameter is just to silence javac about unused variables
: AlreadyClosedException alreadyClosed(RuntimeException unused) {
: - return new AlreadyClosedException("Already closed: " + this);
: + return new AlreadyClosedException("Already closed: " + this, unused);
: }
:
: and added the cause there, which helped me find and fix my wicked
: ways. Is there a reason we decided not to wrap the "unused"
: RuntimeException there?
:
: ---------------------------------------------------------------------
: To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
: For additional commands, e-mail: dev-h...@lucene.apache.org
:
:
-Hoss
http://www.lucidworks.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
--
Uwe Schindler
Achterdiek 19, D-28357 Bremen
https://www.thetaphi.de
eMail: u...@thetaphi.de
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
