Proposal: `finally` block exception suppression

[some-java-user-99206970363698485155 at vodafonemail.de]

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.