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
