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