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.

@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.

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:

    } catch (Throwable t) {
      IOUtils.closeWhileSuppressingExceptions(t, docIn, posIn, payIn);
      throw t;
    }

It would be much nicer like this:

    } catch (Throwable t) {
      throw IOUtils.closeWhileSuppressingExceptions(t, docIn, posIn, payIn);
    }

This can be done by using generics on the method:

  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.

P.S.: Here is the bytecode of the Expression patching: #14602 (comment)

Small note: As this is generated bytecode the generics on the static patcher method are not needed, so left out.

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

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.

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 the generics. Looks like that's not fully solved by the compiler people. 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....).

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 through the method it is no longer final/effectivly final. The method could change the type of exception and return something else while still assignment compatible with regards to the generics.

+1 to eliminate the crazy success pattern! I hope we can also eliminate even the success2 cases :)

Thank you @thecoop for fixing this!

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?

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.