On 21/04/2022 7:30 am,
some-java-user-99206970363698485...@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
