On 11/24/2015 06:38 PM, Martin Buchholz wrote:
Peter: Thanks. I finally got around to thinking about this. Your
argument against mutating the source Future is a good one. Even
though the extra information will prove useful, you've convinced me
that changing the past in this way is going to lead to trouble (like
time travel always does in the movies!).
Better behavior seems: always fail with the exception from the failed
action, and add the source exception to that as a suppressed
exception. This conveys the same information, but with the exceptions
trading places. Propagating the source exception in preference to the
action exception seems wrong because it's the action's job to handle
any Throwable, hence propagation should not be the default. The only
downside is that this is slightly less compatible. Any one who was
relying on being able to extract the source exception in this case
(unlikely!) will still be able to do that, but will need to change
their code to get the suppressed exception.
It's a pity that this changes the specification (or at least bends it).
What do you think of exception cloning?
Regards, Peter
On Mon, Nov 23, 2015 at 7:27 AM, Peter Levart <peter.lev...@gmail.com
<mailto:peter.lev...@gmail.com>> wrote:
Hi,
On 11/23/2015 03:12 PM, Doug Lea wrote:
On 11/23/2015 04:54 AM, Peter Levart wrote:
In CompletableFuture.uniWhenComplete method, the possible
exception thrown from
BiConsumer action is added as suppressed exception to the
exception of the
previous stage. This updated exception is then passed as
completion result to
next stage. When previous stage is appended with more than
one asynchronous
continuation:
...then both secondary exceptions are added as suppressed
to the same primary
exception:
This is not nice for two reasons:
- Throwable::addSuppressed is not thread-safe
- The consumer of the result of one CompletableFuture can
see the exceptional
result being modified as it observes it.
Thanks. The minimal solution is to lock, at least avoiding
conflict
among multiple whenCompletes:
! else if (x != ex)
! x.addSuppressed(ex);
--- 771,781 ----
! else if (x != ex) {
! synchronized (x) {
! x.addSuppressed(ex);
! }
! }
This is not as good a solution as your proposal to add
Throwable.clone(),
but we should do this until something like clone is in place.
Sorry for confusion. I now noticed that Throwable.addSuppressed()
and getSuppressed() methods are actually synchronized! I don't
know how I missed that...
So above patch is not needed. Even printing uses getSuppressed()
that returns a snapshot. So the only thing remaining is the
behavior that exceptions can change while they are being observed.
If this is acceptable then this whole report of mine is just a
bunch of misinformation.
Regards, Peter
When this stage is complete, the given action is invoked
with the result (or
null if none) and the exception (or null if none) of this
stage as arguments.
The returned stage is completed when the action returns.
If the supplied action
itself encounters an exception, then the returned stage
exceptionally completes
with this exception unless this stage also completed
exceptionally."
Could specification be tweaked a bit? The last statement
leaves it open to what
actually happens when "this stage also completes
exceptionally".
The looseness was intentional. We'd like to improve
debuggability of
implementations without strictly promising a particular form
in interfaces.
This is similar to what's done in ForkJoinTask, where we try to
relay exception causes from other threads, but can't promise
anything
beyond a plain exception.
-Doug
