Proposal: `finally` block exception suppression
Joseph D. Darcy
joe.darcy at oracle.com
Thu Jul 14 05:37:04 UTC 2022
Besides the compatibility concerns of changing the operational semantics
of code that has been written for year or even decades, IMO any
investigation of a feature in this vein should judge how often
suppressedExceptions are looked at today. JDK 7 with try-with-resources
shipped in July 2011 so there has been ample time for developers to take
advantage of the suppressed exceptions information.
If few people have, that would weigh further against a proposal to
redefine the long-standing meaning of try-finally.
-Joe
On 7/10/2022 12:35 PM,
some-java-user-99206970363698485155 at vodafonemail.de wrote:
> This is not a formal JEP because I am not a OpenJDK member, and also because I would
> still like to get some feedback on this proposal. But it would be great if you could
> create a JEP if you consider this to be a useful proposal.
> This proposal originated from https://mail.openjdk.org/pipermail/core-libs-dev/2022-April/089016.html
> Thanks a lot already for David Holmes' initial feedback.
>
>
> # Summary
>
> Add automatic exception suppression when exceptions are thrown by `finally` blocks
> to preserve exceptions which occurred in `try` or `catch` blocks.
>
>
> # Description
>
> If exception E1 causes a `finally` block to be executed and the `finally` block is exited due
> to a thrown exception E2 (E2 may be the same as E1), then E1 is added as suppressed exception
> to E2.
>
> E1 can be an exception which occurred in the `try` block, or any of the `catch` blocks.
>
> This requires only changes in the compiler, the language reference and the tests.
> The language grammar is not affected.
>
> The behavior would be the same as the one shown by this code, where every line prefixed
> with # would be the code generated by the compiler:
> ```
> # Throwable e1 = null;
> try {
> # try {
> ... // content of try block
> # } catch (Throwable t) {
> # e1 = t;
> # throw e1;
> # }
> } catch (...) {
> # e1 = null;
> # try {
> ... // content of catch block
> # } catch (Throwable t) {
> # e1 = t;
> # throw e1;
> # }
> } finally {
> # try {
> ... // content of finally block
> # } catch (Throwable e2) {
> # if (e1 != null) {
> # e2.addSuppressed(e1);
> # }
> # throw e2;
> # }
> }
> ```
>
>
> # Rationale
>
> Despite the existence of the try-with-resources statement which properly handles exception
> suppression, there are still situations where a regular try-(catch-)finally statement has
> to be used for clean-up actions, for example to call Lock.unlock().
> Usually the `try` block contains the main business logic and the `finally` block contains
> code for cleaning up resources or restoring a previous state. Therefore an exception which
> occurs in the `try` block is usually important to debug any application failures.
>
> The problem is that if the `finally` block is exited because an exception is thrown, then
> the original exception from the `try` block is lost; it is not even added as suppressed
> exception. A very simplified example for this is the following:
> ```
> try {
> throw new Exception("try");
> } finally {
> throw new Exception("finally");
> }
> ```
> (in more realistic code, instead of the `throw` statements there would be method calls which
> might throw exceptions)
>
> Here the original exception `new Exception("try")` is completely lost. This can make
> debugging or reconstructing failures extremely difficult.
>
> This issue was also mentioned in the original try-with-resources / automatic resource
> management proposal (https://mail.openjdk.org/pipermail/coin-dev/2009-February/000011.html).
> However, back then it appears no proposal was made to adjust the behavior of `finally`
> blocks, most likely because suppressed exceptions was only an optional feature initially.
> (Note that I have not looked at all mails from the coin-dev mailing list, so I might have
> missed a discussion about this.)
>
>
> # Risks / considerations
>
> The proposed change would affect all source code which is recompiled; already compiled
> code would not be affected.
>
> - Noticeable behavior change
> The behavior change described in this proposal would be noticeable to applications
> which access the suppressed exceptions explicitly or implicitly (e.g. by printing
> the stack trace). However, it is unlikely that applications expect certain suppressed
> exceptions, especially since using expressions for regular control flow is discouraged.
>
> - Memory usage impact
> The proposed change could have a performance impact because it would keep the exception
> thrown by the `try` or one of the `catch` blocks alive for a longer time. However,
> due to the improved debuggability this provides, that might be acceptable.
>
> - Execution time impact
> The proposed change might have a negative effect on the execution time of `try` statements
> with `finally` blocks, possibly also negatively affecting the analysis performed by the
> JIT compiler.
> javac could however omit the exception suppression if the statements in the `finally`
> block are guaranteed to never throw exceptions.
>
> - `addSuppressed` call causing StackOverflowError and OutOfMemoryError
> The call to `addSuppressed` in the `finally` block could trigger a StackOverflowError
> or an OutOfMemoryError. This would cause all previous exceptions to be lost.
> The StackOverflowError could possibly be avoided by using @ReservedStackAccess
> (introduced by JEP 270)?
>
> - Self suppression
> Currently the proposal allows E1 and E2 to be the same exception instance. However,
> calling `e.addSuppressed(e)` is forbidden and will cause an IllegalArgumentException
> to be thrown. Such a situation could occur when the thrown exception had been stored
> in a field and is then rethrown by the clean-up actions.
> Not checking for self-suppression and accepting the risk of an IllegalArgumentException
> would match the current behavior of the try-with-resources statement.
> It should be discussed if that behavior is desired. At least the IllegalArgumentException
> is currently constructed with the self-suppressed exception as cause, so it is not "lost".
>
>
> # Related issues / code segments
>
> - https://bugs.openjdk.org/browse/JDK-4988583
> Seems to be the same as this proposal, but was (erroneously?) closed when suppressed
> exceptions were added, even though suppressed exceptions on their own do not solve
> this issue.
> - https://bugs.openjdk.org/browse/JDK-5108147
> Suggests the addition of a `finally (Throwable) { ... }`. With such a construct
> it would at least be easier to write code which manually suppress exceptions
> which occur in the `finally` block.
> - https://github.com/openjdk/jdk/blob/d53b02eb9fceb6d170e0ea8613c2a064a7175892/src/java.base/share/classes/java/util/stream/Streams.java#L837-L887
> Acts very similar to what is described in this proposal, except that there the
> exception from the `try` block is considered the "main" exception, and exceptions
> from the `finally` block are added as suppressed ones.
> - https://github.com/openjdk/jdk/blob/d53b02eb9fceb6d170e0ea8613c2a064a7175892/src/java.net.http/share/classes/jdk/internal/net/http/websocket/WebSocketImpl.java#L737-L763
> Acts similar except that the resulting exception is not rethrown but logged.
> - https://bugs.openjdk.org/browse/JDK-7172206
> Affected by exactly the flaw outlined in the rationale of this proposal.
> - https://bugs.openjdk.org/browse/JDK-8267532
> Optimization problems with try-finally.
>
>
> # Further notes
>
> These notes are not part of the proposal itself, but highlight related issues.
> Some of them had been described in https://mail.openjdk.org/pipermail/core-libs-dev/2022-April/088435.html
> and the replies to it.
>
> - Which expression should be rethrown, which should be suppressed?
> Currently the proposal suggests to keep the behavior of treating the exception from
> the `finally` block as "main" exception, and adding the exception from the `try` or
> `catch` block as suppressed exception.
> However, as also highlighted during the discussion for the try-with-resources statement,
> the exception from the `try` or `catch` block is most likely the more important exception,
> whereas the exception from the `finally` block is not as important. In fact the exception
> from the `finally` block might have only occurred due to the original failure in the
> `try` block which had left the application in an inconsistent state.
> The try-with-resources statement behaves like this and treats the exceptions from the
> `close()` invocation as suppressed. Maybe it would be worth to consider whether the
> behavior of `finally` should be changed to match this, but that might cause backward
> compatibility issue.
>
> - Other control flow statements in `finally`
> The `finally` clause has similar issues when it is exited with other control flow
> statements, such as `return`, `break` or `continue`. For all of these the original
> exception would be lost, even with the changes of this proposal. Additionally, these
> statements can also "overrule" other control flow statements in the body of `try` or
> `catch`:
> ```
> while (...) {
> try {
> // Has no effect
> return 0;
> } finally {
> continue;
> }
> }
> ```
>
> - Further enhancements for exception handling
> There are other exception handling situations which currently require rather verbose
> code, most notably:
> - Given an existing exception E1 (for example from a `catch` clause) and an action
> which has to be executed but may throw an exception:
> Perform the action and add any exception E2 (of specific type?) thrown by that
> action as suppressed exception to E1 and throw E1.
> (Possibly also a variant where E1 may be null in which case E2 is thrown.)
>
> - Given multiple actions which all have to be executed but may throw exceptions:
> Execute all of them, record the first occurred exception E1 and add any subsequent
> exception E2 as suppressed one to E1. Then at the end throw E1.
>
> See also https://bugs.openjdk.org/browse/JDK-8073382.
> However, such changes would require language changes or introduction of new utility
> classes and methods.
>
More information about the amber-dev
mailing list