RFE Pre-review: Support for cloning exceptions

[Martin Buchholz]

I very much want object copying to be simple and easy, but Cloneable has
been under a cloud for a very long time and Effective Java Item 11 advises
to stay away from it.

Josh writes: """Given all of the problems associated with Cloneable, it’s
safe to say that other interfaces should not extend it, and that classes
designed for inheritance (Item 17) should not implement it. Because of its
many shortcomings, some expert programmers simply choose never to override
the clone method and never to invoke it except, perhaps, to copy arrays."""

I don't think we have any history of introducing Cloneable into an
inheritance hierarchy where it was not present before.  Most Throwable
subclasses are "mostly" immutable, but there is no rule that they cannot
have mutable state, which may be corrupted by cloning+mutating, as
explained in Item 11.

Technically, I think you'll need to provide a synchronized clone method on
Throwable to prevent a data race, although an actual bug may be impossible
to reproduce.  We'd like to have Throwable.clone declared to return
Throwable, but that would break subclasses that implemented Cloneable where
clone returned Object.


On Tue, Dec 1, 2015 at 3:22 AM, Peter Levart <peter.levart at gmail.com> wrote:

> Hi,
>
> There are at least two places in java.util.concurrent where it would be
> beneficial if java.lang.Throwable was Cloneable:
>
> - ForkJoinTask::getException() returns original exception thrown by the
> computation of the task when the task is completed exceptionally. The same
> exception is re-thrown in ForkJoinTask::join() or ForkJoinTask::invoke().
> In order for the re-thrown exception to contain meaningful and
> non-misleading stack-trace, the original exception is attempted to be
> replaced with the exception of the same type, with original exception
> attached as the cause, so both stack-traces are visible - the original
> stack trace and the stack-trace of the thread executing join() or invoke().
> In order to do that, ForkJoinTask resorts to using reflection and trying to
> construct new exception by invoking a constructor on the j.l.Class of the
> original exception. It 1st tries the constructor taking j.l.Throwable
> parameter (assumes it will be the cause) and if that doesn't work, it tries
> the no-arg constructor followed by calling initCause() on the result.
>
> This usually works for public exceptions with suitable public
> constructors, but is not guaranteed. So in case it doesn't work, it simply
> re-throws the original exception with the original stack-trace, which hides
> the point at which it was re-thrown (at join() or invoke()). I assume this
> will become more problematic with jigsaw where constructors of non-exported
> exceptions will become inaccessible.
>
> - CompletableFuture::whenComplete[Async]() are methods that return: "...a
> new CompletionStage with the same result or exception as  this stage, that
> executes the given action when this stage completes...". Given 'action' is
> a BiConsumer receiving the result or exception from 'this' stage, so it can
> act as a clean-up action. If this cleanup throws an exception, it becomes
> the result of the returned stage unless 'this' stage also completes with
> exception. Like in try-with-resources, the exception thrown in the body of
> try-with-resources statement has precedence over clean-up exception.
> Clean-up exception is added as suppressed exception. In CompletableFuture
> this presents a problem, because adding a suppressed exception to the
> exception of previous stage effectively modifies the result of the previous
> stage that has already completed. This is undesirable.
>
> So I would like to ask for feedback on a proposal to add cloning support
> to java.lang.Throwable and also how to proceed if this turns out to be
> acceptable (perhaps a CCC request?).
>
> The proposal is as follows:
>
> - add "implements Cloneable" to the j.l.Throwable
>
> - add the following public static method to j.l.Throwable:
>
>
>     /**
>      * Returns a {@link Object#clone() clone} of given {@code exception}
>      * which shares all state with original exception (shallow clone) and
> is
>      * augmented in the following way:
>      * <p>
>      * If {@code resetCause} parameter is {@code true}, then clone's
>      * {@link #getCause() cause} is reset to an uninitialized state so it
> can be
>      * {@link #initCause(Throwable) initialized} again. If {@code
> resetCause}
>      * parameter is {@code false}, then clone's cause is inherited from
> original
>      * exception (either initialized or uninitialized).
>      * <p>
>      * If {@code resetSuppressed} parameter is {@code true} and original
> exception
>      * has suppression enabled, then clone's suppressed exceptions are
> cleared.
>      * If {@code resetSuppressed} parameter is {@code false}
>      * (or original exception has suppression disabled) then clone's
>      * suppressed exceptions are inherited from original exception (or
> clone's
>      * suppression is disabled too). In either case, clone's suppressed
>      * exceptions are independent of original exception's suppressed
>      * exceptions. Any further {@link #addSuppressed(Throwable) additions}
> to
>      * the clone's suppressed exceptions do not affect original exception's
>      * suppressed exceptions and vice versa.
>      *
>      * @param exception       the exception to clone.
>      * @param <T>             the type of exception
>      * @param resetCause      if {@code true}, clone's cause is reset to an
>      *                        uninitialized state.
>      * @param resetSuppressed if {@code true} and original exception has
> suppression
>      *                        enabled, clone's suppressed exceptions are
> cleared.
>      * @return shallow clone of given exception augmented according to
> passed-in
>      *         flags.
>      * @since 1.9
>      */
>     @SuppressWarnings("unchecked")
>     public static <T extends Throwable> T clone(T exception,
>                                                 boolean resetCause,
>                                                 boolean resetSuppressed) {
>         try {
>             synchronized (exception) {
>                 Throwable clone = (Throwable) exception.clone();
>                 if (resetCause) {
>                     // reset to uninitialized state
>                     clone.cause = clone;
>                 }
>                 if (clone.suppressedExceptions != null &&
>                     clone.suppressedExceptions != SUPPRESSED_SENTINEL) {
>                     // suppressedExceptions has already been added to
>                     // and suppression is enabled
>                     clone.suppressedExceptions = resetSuppressed
>                         ? new ArrayList<>()
>                         : new ArrayList<>(clone.suppressedExceptions);
>                 }
>                 return (T) clone;
>             }
>         } catch (CloneNotSupportedException e) {
>             throw new InternalError(e);
>         }
>     }
>
>
>
> In ForkJoinTask the code to construct the re-thrown exception could be
> reduce to:
>
> Throwable original = ...;
>
> Throwable rethrown = Throwable.clone(original, true,
> true).fillInStackTrace().initCause(original);
>
>
> In CompletableFuture::whenComplete[Async] the exceptional result of the
> new stage in case of both original and cleanup exceptions could be computed
> as:
>
> Throwable original = ...;
> Throwable cleanup = ...;
>
> Throwable result = Throwable.clone(original, false, false);
> result.addSuppressed(cleanup);
>
>
>
> So what do you think of adding such feature and do you see any problems
> with it?
>
>
> Regards, Peter
>
>
>