You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "thecoop (via GitHub)" <gi...@apache.org> on 2025/05/09 10:32:18 UTC
[PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
thecoop opened a new pull request, #14633:
URL: https://github.com/apache/lucene/pull/14633
The use of a boolean `success` parameter is common in the Lucene codebase. This can be replaced with a `catch (Throwable t) {...; throw t}` pattern that means a boolean doesn't need to be used at all, and it generally results in less code. This is helped by some new methods on `IOUtils`.
As a side-effect, any problems that occur whilst closing/deleting during exception handling will now be added to the top-level rethrown exception, rather than being swallowed and disappearing.
I've converted a few here to show the pattern - if this is a direction we want to go in, I can convert more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #14633:
URL: https://github.com/apache/lucene/pull/14633#discussion_r2081556092
##########
lucene/backward-codecs/src/java/org/apache/lucene/backward_codecs/lucene101/Lucene101PostingsReader.java:
##########
@@ -120,23 +119,21 @@ public Lucene101PostingsReader(SegmentReadState state) throws IOException {
expectedPayFileLength = -1;
}
CodecUtil.checkFooter(metaIn, null);
- success = true;
} catch (Throwable t) {
- if (metaIn != null) {
- CodecUtil.checkFooter(metaIn, t);
- throw new AssertionError("unreachable");
- } else {
- throw t;
- }
- } finally {
- if (success) {
- metaIn.close();
- } else {
- IOUtils.closeWhileHandlingException(metaIn);
+ try {
+ if (metaIn != null) {
+ CodecUtil.checkFooter(metaIn, t);
+ throw new AssertionError("unreachable");
+ } else {
+ throw t;
+ }
+ } catch (Throwable ct) {
+ IOUtils.closeWhileSuppressingExceptions(ct, metaIn);
+ throw ct;
}
}
+ metaIn.close();
Review Comment:
Can we not use try with resources here?
##########
lucene/core/src/java/org/apache/lucene/util/IOUtils.java:
##########
@@ -247,6 +308,33 @@ public static void deleteFilesIgnoringExceptions(Collection<? extends Path> file
}
}
+ /**
+ * Deletes all given files, ignoring all thrown Throwables.
+ *
+ * <p>Some of the files may be null, if so they are ignored.
+ */
+ public static void deleteFilesSuppressingExceptions(Throwable ex, Path... files) {
+ deleteFilesSuppressingExceptions(ex, Arrays.asList(files));
+ }
+
+ /**
+ * Deletes all given files, ignoring all thrown Throwables.
Review Comment:
Add to docs that the suppressed exceptions are recorded in the given Throwable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866336273
Hi,
In general at the time when the success code was written a catch/rethrow of Throwable would not compile without an inappropriate throws clause on the method. Because the java compiler got better in type tracking, the rethrow would not impose that a Throwable needs to be declared on the method declaration, as the compiler knows which checked exceptions could appear in the catch clause and can restrict types of possible checked exceptions.
So in general I am +1 to do this. Code is cleaner and the suppressed exceptions are also much better.
I have not checked modified logic in the PR, but the overall direction looks fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866484934
@uschindler PR updated. I've applied this refactor to the most recent set of formats, as those are most likely to be copied in future format changes. That seems like a good place to leave this first PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on code in PR #14633:
URL: https://github.com/apache/lucene/pull/14633#discussion_r2081616824
##########
lucene/backward-codecs/src/java/org/apache/lucene/backward_codecs/lucene101/Lucene101PostingsReader.java:
##########
@@ -120,23 +119,21 @@ public Lucene101PostingsReader(SegmentReadState state) throws IOException {
expectedPayFileLength = -1;
}
CodecUtil.checkFooter(metaIn, null);
- success = true;
} catch (Throwable t) {
- if (metaIn != null) {
- CodecUtil.checkFooter(metaIn, t);
- throw new AssertionError("unreachable");
- } else {
- throw t;
- }
- } finally {
- if (success) {
- metaIn.close();
- } else {
- IOUtils.closeWhileHandlingException(metaIn);
+ try {
+ if (metaIn != null) {
+ CodecUtil.checkFooter(metaIn, t);
+ throw new AssertionError("unreachable");
+ } else {
+ throw t;
+ }
+ } catch (Throwable ct) {
+ IOUtils.closeWhileSuppressingExceptions(ct, metaIn);
+ throw ct;
}
}
+ metaIn.close();
Review Comment:
Yes - needs some tweaks to the try structures...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866538943
Unfortunately that doesn't work here - Java forgets the specific exception types thrown in the try when the exception goes in & out of the generic method - it infers `T` to `Throwable`, and now the method returns (and so throws) `Throwable`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866550770
> Unfortunately that doesn't work here - Java forgets the specific exception types thrown in the try when the exception goes in & out of the generic method - it infers `T` to `Throwable`, and now the method returns (and so throws) `Throwable` as the raw type
That is strange..... I have to dig further. I did this a while ago in a similar way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2867787360
+1 to eliminate the crazy `success` pattern! I hope we can also eliminate even the `success2` cases :)
Thank you @thecoop for fixing this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866504083
P.S.: Here is the bytecode of the Expression patching: https://github.com/apache/lucene/pull/14602#issuecomment-2845328503
Small note: As this is generated bytecode the generics on the static patcher method are not needed, so left out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty merged PR #14633:
URL: https://github.com/apache/lucene/pull/14633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866499124
I have a suggestion. I used the same approach in the expressions module to patch the stack trace and rethrowing exceptions (to include the source code of the exception into the stack trace). So basically at moment you always use code like this:
```java
} catch (Throwable t) {
IOUtils.closeWhileSuppressingExceptions(t, docIn, posIn, payIn);
throw t;
}
```
It would be much nicer like this:
```java
} catch (Throwable t) {
throw IOUtils.closeWhileSuppressingExceptions(t, docIn, posIn, payIn);
}
```
This can be done by using generics on the method:
```java
public static <T extends Throwable> T closeWhileSuppressingExceptions(T ex, Iterable<? extends Closeable> objects) {
for (Closeable object : objects) {
try {
if (object != null) {
object.close();
}
} catch (Throwable e) {
ex.addSuppressed(e);
}
}
return ex;
}
```
This allows the compiler to still take record of the exceptions and the code looks much better because you actually see that the method modifies the exception.
If you don't want to use the return type, you can still rethrow the conventional way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "thecoop (via GitHub)" <gi...@apache.org>.
thecoop commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2873415595
I definitely agree that having to have a separate `throw t` as part of the exception handling is trappy (although generally ok due to flow-control changes breaking the compile without it), and I would prefer to have a single line or use try-resources with lambdas in some way, ideally without putting the whole try block in a lambda. I'll have a play around with it & see what happens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866618642
The reason for that problem is in the JLS: https://docs.oracle.com/javase/specs/jls/se24/html/jls-11.html#jls-11.2.2
> A throw statement whose thrown expression is a final or effectively final exception parameter of a catch clause C can throw an exception class E iff:
>
> * E is an exception class that the try block of the try statement which declares C can throw; and
> * E is assignment compatible with any of C's catchable exception classes; and
>
> * E is not assignment compatible with any of the catchable exception classes of the catch clauses declared to the left of C in the same try statement.
The problem is: When it passes the method it is no longer final/effectivly final. The method could change the type of exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2866593107
Indeed that does not work. I was under the impression that the compiler should be able to see that the method parameter has same type as return type by this. Looks like that's not fully solved. Especally as it gets harder for multi-catch.
I thought it did not work with multi-catch, but that it works here. Of course you could make a more horrible hack, but some people don't like this in the Lucene community (its known as sneaky rethrow....).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Catch and re-throw Throwable rather than using a success boolean [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #14633:
URL: https://github.com/apache/lucene/pull/14633#issuecomment-2869000472
>
> I thought it did not work with multi-catch, but that it works here. Of course you could make a more horrible hack, but some people don't like this in the Lucene community (its known as sneaky rethrow....).
But what if the horrible hack (sneakythrow) could be contained to just be inside these utility methods (IOUtils), to simplify exception handling elsewhere?
IMO this would be worth it, even if it saves just one line at these call sites. Historically the exception handling code has been problematic (easy to leak files or mask important information, accidentally suppress the wrong thing, etc etc)...
Maybe we should look into it as a followup?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org