Improve `finally` block exception handling

[David Holmes]

On 21/04/2022 7:30 am, 
some-java-user-99206970363698485155 at vodafonemail.de wrote:
> Thanks a lot for your feedback! My original message was a bit vague in parts, sorry for that.
> 
> The proposal consists of the following:
> 1. Forbidding usage of `break`, `continue`, `yield` and `return` in `finally` blocks
> 2. Adding exceptions as suppressed exceptions:
>     If exception E1 led to execution of the `finally` block and the block is left due to
>     another exception E2, then either E1 or E2 should be thrown with the other one added
>     as suppressed exception. For consistency with try-with-resources and because E1 is
>     most likely more relevant ideally E1 would be thrown and E2 should be suppressed. But
>     if you think the impact on backward compatibility would be too large, then E2 should
>     be thrown (the current behavior), but E1 should at least be added as suppressed exception.
> 
> The following replies to your comments hopefully make the rationale for these proposed
> changes clearer.
> 
> 
>> The behaviour of try/catch/finally is not "obvious at all", you have to read what it means
>> and when you do that you clearly see what happens regarding exceptions.
> 
> For try-catch you see that the code catches some specific exception and then handles it in a
> certain way, whether that is by rethrowing, logging or itentionally ignoring it.
> The issue with `finally` is that in its current form, exceptions which occurred in the `try`
> block might just silently disappear. Consider this simple example:
> ```
> try {
>      throw new RuntimeException();
> } finally {
>      return true;
> }
> ```
> Here it is not clear at all, unless you have read the JLS in detail, that the thrown exception
> just vanishes. There is no explicit indication that the `finally` has any effect on the
> thrown exception. Of course this is a contrived example, but consider the same situation where
> the `try` block calls a method which might throw an exception (or the general case that a
> VirtualMachineError occurs), and that the `return` (or any of the other affected statements)
> is not the only statement in the `finally` block.
> 
> Similar confusing code can be written where the `try` or `catch` block returns a value
> (or continues or breaks loop iteration), but the `finally` block overwrites that action:
> ```
> try {
>      return 1;
> } finally {
>      return 2;
> }
> ```
> Again, contrived, but consider the same code with additional statements in the `try` and
> `finally` blocks. This breaks fundamental assumptions one has about the behavior of the
> statements, such as `return`.

I still maintain this is simply a matter of user education. You have no 
business using a finally block if you have never bothered to even learn 
how it works. Sorry no sympathy from me on that one.

>>> Are there any plans to forbid usage of `break`, `continue`, `yield` and `return` in
>>> `finally` blocks?
>>
>> Why would we do that? What would that gain?
> 
> It would prevent code such as the one shown above, where code, most likely unintentionally,
> discards exceptions. Now also consider that inside the `try` block a VirtualMachineError
> (or a subclass of it) may occur which you really should not catch. Yet the code in the
> `finally` block silently discards it without you ever noticing the error, before it occurs
> later in another part of the application again and has possibly already corrupted the state
> of the application.

Again this is primarily user education. And there are times when you 
definitely do want to use those statements and you would have to jump 
through hoops to get the same effect if they were banned.

But in terms of "are there any plans ..." the answer is a clear no as 
the compatibility issues would make this a non-starter.

> 
>>> Similarly for `throw` and any implicitly thrown exceptions, is there a plan to at least
>>> add the original exception as suppressed one to the newly thrown?
>>
>> That suggestion is not completely without merit, but nor is it a "slam-dunk" obvious thing to do. The semantic implications of the exceptions matter, and semantics come from the programmers intent. There's no reasonable way to automagically determine that when an exception is created that another exception (that led to the finally block) should be inserted as a "suppressed exception". That would actually be completely wrong to do in many circumstances you would instead need to act when the exception would terminate the finally block and then add the original exception as the "suppressed" exception.
> 
> To clarify my suggestion: If a `finally` block is entered due to an exception E1, and is
> exited due to an exception E2 (regardless of whether explicitly thrown by a `throw` statement),
> and E1 != E2, then both exceptions should be preserved, with one being added as suppressed
> exception to the other one.

That is not unreasonable: E2 suppresses E1.

> 
>> But really the programmer is in the best position to decide how exceptions need to be handled.
> 
> Except that in a `finally` block they don't have access to the exception which led to execution
> of the `finally` block, unless they write verbose hacky code to first have a `catch (Throwable)`
> which stores the caught exception in a local variable. To me it appears for many `finally`
> blocks in existing code it was not actively decided how exception handling should be done,
> but rather it was forgotten that the original exception might get discarded when the `finally`
> throws a new exception; or that behavior was considered acceptable because currently Java does
> not allow you to handle it in a better way (which is one of the main points of this proposal).

The programmer writing the finally block also wrote, or has access to 
the try block that allows the original exception, so they are still in 
the best position to determine how any given chunk of code should react 
to exceptions IMO.

>>> Of course one could argue that the same applies to `catch` clauses whose body accidentally
>>> causes an exception which discards the caught one. However the main difference is that
>>
>> Yes exactly the same.
> 
> This point was mainly a safeguard in my proposal to avoid someone extending the scope of
> this proposal to `catch` clauses as well, which then could cause this proposal as a whole
> to be turned down. Therefore I am explicitly not arguing for any changes to `catch` clauses
> exception handling / suppression here, because there it is at least somewhat more obvious
> what is going on with the exceptions.
> 
>>> there, only a specific exception type is caught (and discarded), whereas for `finally`
>>> exceptions of _any_ type are discarded. It could also be argued that adding suppressed
>>
>> We have multi-catch now so that argument is somewhat weaker.
> 
> The point here was that you explicitly write the exception type, whether that is a single type
> or a union type in case of a multi-catch, in constrast to how `finally` discards any exception
> type without you explicitly naming the type.
> 
>>> exceptions decreases performance or causes memory leaks, but one the other hand
>>> this behavior was explicitly included try-with-resources statements, most likely because the
>>> value this adds was considered more important than any performance issues this
>>> might cause.
>>
>> try-with-resources added support for suppressed exceptions because the automatic closing of the resource could throw an exception, and that had to be factored in to the whole mechanism.
> 
> Yes, but I think the scenarios are exactly identical:
> try-with-resources:
>    1. Original exception E1 occurs
>    2. Resource cleanup is done, here through implicit `close()`
>    3. `close()` throws an exception E2
>    4. E2 is added as suppressed to E1
> 
> Current `finally` block behavior:
>    1. Original exception E1 occurs
>    2. Resource cleanup is done, here manually in `finally` block
>    3. `finally` block throws an exception E2
>    4. !! E1 is silently lost !!
> 
> Why is it here acceptable to lose the original exception E1, even though the use
> cases are exactly the same?

try-with-resources (twr) had to introduce suppressed exceptions to deal 
with the fact that the majority of close() operations throw a checked 
IOException, so twr had to deal with that. If trw had been implemented 
as a simple try/finally then the close() exception is the one that would 
be thrown. The designers of twr didn't want that because they decided 
that the exception from the try block was probably much more important 
to be seen in this context, than the IOException from the close(). So 
they introduced suppressed exceptions and defined twr such that the 
exception from the try block always gets rethrown by suppressing any 
exception from the finally/close(), and that exception from the close() 
is available through suppressed exception API.

So in the specific context of twr the original exception E1 was 
considered too important relative to the IOException (E2) from close() 
to allow it to be lost. So they worked around the behaviour of try/finally.

Given that we now have suppressed exceptions I agree there is merit in 
reusing it outside twr to allow E1 to not be lost when E2 occurs. But it 
would be suppression in the opposite sense to twr: the finally 
exception, E2, suppresses the try exception, E1.

> I assume the reason why they behave differently is historic, because `finally` blocks
> exist way longer than suppressed exceptions. However, that does in my opinion not
> preclude updating it retroactively, especially when it is unlikely that the difference
> will have a visible negative effect at runtime, but instead makes understanding
> application failures easier.

As I said I agree there is merit to this aspect, but it is not a 
core-libs issue. This requires a language change, and supporting javac 
changes. I suggest taking up this one aspect on the amber-dev mailing list:

https://openjdk.java.net/projects/amber/

I don't know if extending the use of suppressed exceptions came up when 
twr (automatic resource management) was developed under Project Coin:

https://openjdk.java.net/projects/coin/

Cheers,
David

> 
> Kind regards
> 
>