Hi Mike,
here is the PR improving the situation while not making the signalling
NPE visible to code: https://github.com/apache/lucene/pull/12705
In MemorySegmentIndexInput theres also InvalidStateException which
cannot be throws on wrong parameters. This one is also internal and
always mapped to AlreadyClosedException (also without cause).
Uwe
Am 21.10.2023 um 10:23 schrieb Uwe Schindler:
Hi,
I have a good idea: We should only wrap as AlreadyClosedException if
and only if the bytebuffers/memorysegemnts are null (see . In all
other cases rethrow the NPE:
AlreadyClosedException alreadyClosed(RuntimeException npe) {
if (npe == null || this.buffers == null) { // buffers == null if input
closed!
return new AlreadyClosedException("Already closed: " + this);
}
throw npe;
}
(this would need the same change in all MemorySegmentIndexInput in a
similar way).
This would keep the NPE on wrong usage, but in the case of a closed
ByteBufferIndexInput / MemorySegmentIndexInput it would throw plain
AlreadyClosedEx.
I can provide a PR, but give me a week, I am very busy.
Uwe
Am 21.10.2023 um 10:01 schrieb Uwe Schindler:
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
--
Uwe Schindler
Achterdiek 19, D-28357 Bremen
https://www.thetaphi.de
eMail:u...@thetaphi.de
