REASSERT Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed

[David Holmes]

Hi Joe,

On 23/04/2013 9:05 AM, Joseph Darcy wrote:
> Hello,
>
> Just reasserting the request for a review of the latest version of this
> patch:
>
>      http://cr.openjdk.java.net/~darcy/8012044.2
>
> I believe this version does an appropriate job of propagating exception
> information when there is misuse of the methods on Throwable.

I still find the use of addSuppressed in initCause to be questionable. 
Given:

    catch(SomeException s) {
       sharedException.initCause(s); // oops already has a cause
       throw sharedException;
    }

then the ISE isn't suppressing 's', but replacing/suppressing 
sharedException in my view, as it is what would have been thrown had the 
ISE not occurred.

I understand the desire to not lose sight of the fact that 's' was 
thrown, but this is really no different to a finally block losing the 
original exception info. (And fixing that was rejected when the 
suppression mechanism was added.)

Anyway this isn't a "block" (not that such a thing exists), just a 
comment. The change isn't harmful and may be useful.

Cheers,
David

> Thanks,
>
> -Joe
>
> On 4/17/2013 10:32 AM, Joe Darcy wrote:
>> On 04/14/2013 07:36 PM, Joe Darcy wrote:
>>> On 04/12/2013 07:29 PM, Jason Mehrens wrote:
>>>> Joe,
>>>> You'll have guard ise.addSuppressed against null.  Looks good
>>>> otherwise.
>>>>
>>>> private static void initCauseNull() {
>>>>       Throwable t1 = new Throwable();
>>>>       t1.initCause(null);
>>>>       try {
>>>>         t1.initCause(null);
>>>>       } catch(IllegalStateException expect) {
>>>>       }
>>>> }
>>>
>>> Right you are; check added and test updated in:
>>>
>>>     http://cr.openjdk.java.net/~darcy/8012044.2/
>>>
>>> Full patch to Throwable:
>>
>> [snip]
>>
>> One more iteration; I've changed the initCause logic to suppress both
>> exceptions rather than using one as the cause:
>>
>>      http://cr.openjdk.java.net/~darcy/8012044.2
>>
>> Patch to throwable:
>>
>> --- old/src/share/classes/java/lang/Throwable.java    2013-04-14
>> 19:33:23.000000000 -0700
>> +++ new/src/share/classes/java/lang/Throwable.java    2013-04-14
>> 19:33:23.000000000 -0700
>> @@ -452,10 +452,15 @@
>>       * @since  1.4
>>       */
>>      public synchronized Throwable initCause(Throwable cause) {
>> -        if (this.cause != this)
>> -            throw new IllegalStateException("Can't overwrite cause");
>> +        if (this.cause != this) {
>> +            IllegalStateException ise =
>> +                new IllegalStateException("Can't overwrite cause",
>> this);
>> +            if (cause != null)
>> +                ise.addSuppressed(cause);
>> +            throw ise;
>> +        }
>>          if (cause == this)
>> -            throw new IllegalArgumentException("Self-causation not
>> permitted");
>> +            throw new IllegalArgumentException("Self-causation not
>> permitted", this);
>>          this.cause = cause;
>>          return this;
>>      }
>> @@ -1039,7 +1044,7 @@
>>       */
>>      public final synchronized void addSuppressed(Throwable exception) {
>>          if (exception == this)
>> -            throw new
>> IllegalArgumentException(SELF_SUPPRESSION_MESSAGE);
>> +            throw new
>> IllegalArgumentException(SELF_SUPPRESSION_MESSAGE, exception);
>>
>>          if (exception == null)
>>              throw new NullPointerException(NULL_CAUSE_MESSAGE);
>>
>> The suppression mechanism is typically, but not exclusively, used by
>> the try-with-resources statement.
>>
>> Thanks,
>>
>> -Joe
>