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

[David Holmes]

Sorry this slipped through the cracks.

Looks good to me. (Don't know if you already pushed it :) )

David

On 25/04/2013 5:16 PM, Joe Darcy wrote:
> Hello,
>
> Responding to David's comment and some comments from Alan off-list, here
> is a variant which doesn't use suppressed exceptions in initCause, but
> still passes along some information:
>
>      http://cr.openjdk.java.net/~darcy/8012044.4
>
> Patch to Throwable:
>
> --- a/src/share/classes/java/lang/Throwable.java    Wed Apr 24 21:27:52
> 2013 +0000
> +++ b/src/share/classes/java/lang/Throwable.java    Thu Apr 25 00:15:32
> 2013 -0700
> @@ -453,9 +453,10 @@
>        */
>       public synchronized Throwable initCause(Throwable cause) {
>           if (this.cause != this)
> -            throw new IllegalStateException("Can't overwrite cause");
> +            throw new IllegalStateException("Can't overwrite cause with
> " +
> + Objects.toString(cause, "a null"), this);
>           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 +1040,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);
>
> Thanks,
>
> -Joe
>
> On 04/22/2013 10:51 PM, Joe Darcy wrote:
>> Hi David,
>>
>> On 04/22/2013 10:25 PM, David Holmes wrote:
>>> 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.)
>>
>> Project Coin discussions did note try-catch-finally and
>> try-with-resources were inconsistent on this point. While the
>> try-with-resources behavior is regarded as preferable, we thought it
>> would be too large a change to redefine the long-standing semantics of
>> try-catch-finally.
>>
>>>
>>> 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
>>>
>>
>> Yes, I would describe the intention of this change as provding
>> programmers more information to debug when the methods are Throwable
>> and used improperly.
>>
>> Thanks,
>>
>> -Joe
>