Throwable.addSuppressed error conditions -- use the exception as the cause?

[Zhong Yu]

On Tue, Apr 9, 2013 at 11:54 AM, Steven Schlansker
<stevenschlansker at gmail.com> wrote:
>
> On Apr 9, 2013, at 9:30 AM, Zhong Yu <zhong.j.yu at gmail.com> wrote:
>
>>
>>
>>
>> On Mon, Apr 8, 2013 at 6:54 PM, Steven Schlansker <stevenschlansker at gmail.com> wrote:
>> Today I encountered "java.lang.IllegalArgumentException: Self-suppression not permitted" from Throwable.addSuppressed.
>>
>> My first surprise is that the try-with-resources block can throw this exception; it is very confusing to have auto-generated code throw exceptions.  But beyond that, it is impossible to figure out *which* exception caused the problem.  If you have multiple resources acquired in the try-with-resources, it could be any of them.
>>
>> Would it be reasonable to change
>>
>>     public final synchronized void addSuppressed(Throwable exception) {
>>         if (exception == this)
>>             throw new IllegalArgumentException(SELF_SUPPRESSION_MESSAGE);
>>
>> to
>>
>>     public final synchronized void addSuppressed(Throwable exception) {
>>         if (exception == this)
>>             throw new IllegalArgumentException(SELF_SUPPRESSION_MESSAGE, this);
>>
>> so that when you get this exception it at least points to one of the throw sites that actually caused the problem?  Otherwise the only stack trace you have is in auto-generated code and you are left scratching your head, wondering where it came from.  I believe this would increase the debuggability of the try-with-resources construct and there's no immediately obvious downside.
>>
>> I'm not the only one who was confused by this behavior:
>> http://stackoverflow.com/questions/12103126/what-on-earth-is-self-suppression-not-permitted-and-why-is-javac-generating-co
>>
>>
>> Reusing exception is probably not a good idea - unless the exception is immutable, i.e. enableSuppression=writableStackTrace=true.
>>
>
> I agree, but there is (library) code out there that does this -- the code that caused the problem wasn't even mine.

The library should be told to fix the bug. It is really serious, for
example, more and more suppressed exceptions can be added to this
reused/shared/cached exception, causing memory leak. A mutable
exception simply cannot be shared, otherwise there will be multiple
exception handler trying to mutate it.

Zhong Yu

> My suggestion has no cost unless you happen to trigger it, and then it helps you find who is causing trouble.
>
>> Interestingly, even for an immutable exception, `e.addSuppressed(e)` isn't allowed. I think that it should be allowed.
>>
>> Zhong Yu
>