RFR: jsr166 jdk9 integration wave 2

[Martin Buchholz]

On Thu, Dec 10, 2015 at 12:04 AM, Peter Levart <peter.levart at gmail.com> wrote:
> Hi Martin,
>
> On 12/10/2015 12:18 AM, Martin Buchholz wrote:
>>
>> I finally studied ForkJoinTask.getThrowableException - I was not aware
>> that we were doing reflection based exception cloning.  In practice it
>> probably muddles through, but it just seems too hacky to use in a java
>> core library.  We're relying on common conventions, but in principle
>> we are invoking random code with an unknown spec.  The stack traces
>> created are fake in the sense that they are not created naturally, and
>> the thrown exception may differ from the original in significant ways,
>> most notably missing the message string.  With heroic efforts we can
>> make it safer, e.g. examine the bytecode for the constructors we are
>> invoking, and check whether they call the super constructors in the
>> right way, but that will be a major engineering project by itself.
>
>
> I assume the aim of exception cloning in ForkJoinTask.getThrowableException
> is to provide the user with a stack-trace that originates in its own thread.
> This helps trace the origin of the exception in case it's handled way up in
> the call stack. In addition, the original stack trace is also preserved
> (original exception is added as a cause).
>
> This case does not suffer from the visibility of original exception before
> ForkJoinTask.getThrowableException is called, so perhaps the aim could be
> achieved in a slightly different manner: just return the original exception,
> but add a newly constructed CrossThreadPropagationException as a suppressed
> exception to it. Printing such augmented original exception would then
> reveal both stack traces. What do you think?

I like it!  Although we are bending the purpose of suppressed exceptions a bit.

>> For CompletableFuture.whenComplete: I also keep moving in the
>> direction of less magic.  It is perfectly possible for a well-written
>> whenComplete action to catch all Throwables, add the source exception
>> as a suppressed exception, and rethrow.  In fact, perhaps people have
>> already tried this and discovered we incorrectly throw the source
>> exception.  I think we should really accept our mistake and switch to
>> throwing the action exception rather than the source exception.
>> Perhaps we should only call addSuppressed if the source exception is
>> not already in the list of suppressed exceptions.
>
>
> Less is more. I would even not bother with the additional logic of
> conditionally adding source exception to the list of suppressed exceptions.
> I doubt many codes out there do that in the whenComplete handler since such
> exception was ignored up until now. And if anybody does that, she will get
> the source exception listed twice - not a big deal. If javadoc describes the
> behavior, this will not happen frequently.

Perhaps we should even retreat further into "less is more" territory
and trust the programmer, reducing our involvement to friendly advice
in the javadoc.  Version 0.1:

"""We recommend writing whenComplete actions thus:

try {
  ... real code here ...
} catch (Throwable t) {
  t.addSuppressed(source);
 throw t;
}
"""