RFE Pre-review: Support for cloning exceptions

[Peter Levart]

Hi Martin,

I see this is a controversial topic and I would really not like to delay 
changes to CompletableFuture because of that. Anyway I feel I should 
answer to some of your concerns...

On 12/02/2015 07:39 PM, Martin Buchholz wrote:
>
>
> On Wed, Dec 2, 2015 at 1:32 AM, Peter Levart <peter.levart at gmail.com 
> <mailto:peter.levart at gmail.com>> wrote:
>
>
>     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.
>
>
> Java is supposed to be a high reliability platform and introducing new 
> exotic failure modes isn't something we should be doing.  My work at 
> Google prejudices me - it seems half the work we do is trying to track 
> down rare bugs we can't reproduce, or happen once every cpu-year.

I agree.

>
> If you add "implements Cloneable" then people will call protected 
> nullary clone().  You haven't made it public, but it can (and will!) 
> be called from subclasses and their packages.  I see you were careful 
> to copy the suppressedExceptions field in your static clone, but 
> callers of the "real" clone will produce two Throwables that share a 
> single ArrayList, which is not thread safe.

Right. You can never trust users to do the recommended thing even if it 
is written all over the place in big friendly letters. Previous 
iteration had the code in virtual Throwable.clone(). I resorted to sole 
static method to keep behavior of possible Throwable subclasses that 
implement Cloneable themselves, but that behavior would actually be 
improved in that case.

>
> I'm sure there's code out there that reasonably assumes that if 
> something is Cloneable, then it has a public clone() method, and will 
> try to invoke that via reflection.

This change is "observable" and of course there can be code that 
observes just that. If there's code that blindly calls clone() via 
reflection on objects that implement Cloneable, I would expect it to be 
prepared to handle the cases where the implementing class or the method 
is not accessible.

>
> I see that if an existing subclass of Throwable implemented Cloneable, 
> there would be no way (short of Unsafe) for its clone method to do a 
> proper copy of suppressedExceptions.  In the future, if Throwable were 
> to implement Cloneable, then more subclasses are likely to add a 
> public clone() method (to "support" cloning) and that will only do a 
> proper copy of suppressedExceptions if you do the copying in nullary 
> Throwable.clone() instead of in static clone.

This argument is not entirely correct. If cloning of Throwables was 
always invoked via the proposed static method which just delegates to 
vritual Object.clone(), then all overriding clone() methods in 
subclasses would be invoked and the code in static method would then 
copy the suppressed exceptions list. Of course, you have a legitimate 
concern that code could also invoke the virtual clone() directly, so 
this would be better:

     /**
      * Clones this exception by invoking {@link Object#clone()} and if this
      * exception has suppression enabled, also copies the list of
      * {@link #getSuppressed() suppressed} exceptions so that any further
      * {@link #addSuppressed(Throwable) additions}  to
      * the clone's suppressed exceptions do not affect original exception's
      * suppressed exceptions and vice versa. If this exception has 
suppression
      * disabled, the returned clone will have suppression disabled too.
      * @implNote Subclasses that maintain mutable state are recommended to
      * override this method, call {@code super.clone()} and duplicate 
the mutable
      * state so that clone's mutable state is not shared
      * with this instance's. Immutable state need not be duplicated.
      *
      * @return the clone of this exception with copied list of suppressed
      *         exceptions if suppression is enabled.
      * @throws CloneNotSupportedException this method does not throw 
it, but
      *                                    if a subclass wishes to 
opt-out of
      *                                    cloning, it should throw
      *                                    CloneNotSupportedException 
from method
      *                                    that overrides this method. It is
      *                                    recommended that this is not 
performed
      *                                    unless really needed (say a 
subclass
      *                                    wishes to maintain a 
singleton instance
      *                                    invariant for all costs).
      * @since 1.9
      */
     @Override
     protected synchronized Object clone() throws 
CloneNotSupportedException {
         Throwable clone = (Throwable) super.clone();
         if (clone.suppressedExceptions != null &&
             clone.suppressedExceptions != SUPPRESSED_SENTINEL) {
             // suppressedExceptions has already been added to and 
suppression is enabled
             clone.suppressedExceptions = new 
ArrayList<>(clone.suppressedExceptions);
         }
         return clone;
     }

     /**
      * Invokes {@link Throwable#clone()} and modifies the returned clone in
      * the following way before returning it:
      * <p>
      * If {@code resetCause} parameter is {@code true}, then clone's
      * {@link #getCause() cause} is reset to uninitialized state so it 
can be
      * {@link #initCause(Throwable) initialized} again.
      * <p>
      * If {@code resetSuppressed} parameter is {@code true} and 
original exception
      * has suppression enabled, then clone's suppressed exceptions are 
cleared,
      * but since clone maintains it's own list of suppressed 
exceptions, original
      * exception's suppressed exceptions are not affected.
      *
      * @param exception       the exception to clone.
      * @param <T>             the type of exception
      * @param resetCause      if {@code true}, clone's cause is reset to
      *                        uninitialized state.
      * @param resetSuppressed if {@code true} and original exception 
has suppression
      *                        enabled, clone's suppressed exceptions 
are cleared.
      * @return clone of given exception modified according to passed-in 
flags.
      * @throws CloneNotSupportedException if {@link Throwable#clone()} 
throws it.
      * @since 1.9
      */
     @SuppressWarnings("unchecked")
     public static <T extends Throwable> T cloneException(T exception,
                                                          boolean 
resetCause,
                                                          boolean 
resetSuppressed)
         throws CloneNotSupportedException {
         Throwable clone = (Throwable) exception.clone();
         if (resetCause) {
             // reset to uninitialized state
             clone.cause = clone;
         }
         if (resetSuppressed &&
             clone.suppressedExceptions != null &&
             clone.suppressedExceptions != SUPPRESSED_SENTINEL) {
             // clear suppressed exceptions if suppression is enabled
             // and there are already suppressed exceptions added to the
             // copied list
             clone.suppressedExceptions.clear();
         }
         return (T) clone;
     }

>
> Sadly, making Throwable Cloneable is exceedingly subtle and I think we 
> will regret it (as we do the design of Cloneable itself).

I tend to agree. In a perfect world, programmers code carefully and pay 
attention to details, but in real world they need hard constraints.

Thanks for reading and discussing,

Regards, Peter