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`.
>> 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.
>> 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.
> 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).
>> 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?
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.
Kind regards
