RFR: jsr166 jdk9 integration wave 2

[Martin Buchholz]

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.


On Mon, Nov 23, 2015 at 7:27 AM, Peter Levart <peter.levart at 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
>>
>>
>