RFE Pre-review: Support for cloning exceptions

[Peter Levart]

Hi Martin,


On 12/02/2015 06:48 AM, Martin Buchholz wrote:
> 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.

I'm aware of potential problems with clone() method in general. But 
given that exceptions are a special kind of hierarchy which is typically 
kept very simple (they even have a constraint that they can't be generic 
types) and in which most of the times, Throwable subclasses are not 
adding state, and if they do, it is mostly immutable, and given that 
this proposal is about solving the problem some 
parallelizing/asynchronous execution frameworks have or might have with 
propagating exceptions from other threads where the mutable state that 
must be copied or replaced consists solely from the state declared in 
Throwable, I think that in practice this would not pose real problems.

In the two examples given, the alternative or status quo is to propagate 
original exception, which is in both cases worse than using a clone 
constructed with this proposal.

>
> 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.

If code mutates some state in a Throwable subclass, it must be aware of 
whether this state is shared with another instance or not. If it expects 
it is safe to mutate that state, but is given an instance that shares 
that state with another instance, we can get unpredictable behavior.

Exceptions are generally constructed and mutated in one place and then 
treated as objects that don't change state. This particular usage 
pattern minimizes the risks - the code that mutates the state of an 
exception would usually be aware of where it comes from and whether it 
is safe to do that.

>
> 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.

The proposal is adding a static method and not overriding 
Object.clone(). This is to keep behavior of possible Throwable 
subclasses that already implement Cloneable. The static method is just 
invoking clone() and is synchronizing on the original exception. This is 
to make sure, Throwable part of original exception's state is not 
mutated while it is being cloned. That state consists only of the list 
of suppressed exceptions. Subclasses that wanted to copy their mutable 
subclass state when cloned, would have to do their own synchronization. 
There's nothing a synchronized instance Throwable.clone() could do to 
help them. It would be tempting to override Object.clone() in Throwable 
and make it final, but that would break any possible existing subclasses 
that override clone().

The only thing I would change in the proposal is the handling of 
CloneNotSupportedException. I would not handle it. If a Throwable 
subclass wishes to prevent cloning of it's instances, it could override 
clone() and throw CloneNotSupportedException. This would give subclasses 
a possibility to opt-out. For example, some exceptions might be designed 
to be immutable singletons for some reason. Users of static 
Throwable.clone() would have to respect that (probably by passing the 
unchanged original exception on).

In short, I think exceptions are a special hierarchy with special use 
pattern in which clone() would not present a practical problem that 
generally arises in other objects that are meant to change state 
independently from their creation.

Regards, Peter

>
>
> On Tue, Dec 1, 2015 at 3:22 AM, Peter Levart <peter.levart at gmail.com 
> <mailto: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
>
>
>