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.lev...@gmail.com <mailto:peter.lev...@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




Reply via email to