Request for review: 7084245: Update usages of InternalError to use exception chaining

[Sebastian Sickelmann]

Sorry i have forgotten the webrev url.

http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_main_1/



Am 30.08.2011 10:20, schrieb Sebastian Sickelmann:
> Am 29.08.2011 21:10, schrieb Xueming Shen:
>> Hi Sebastian,
>>
>> I will help to push the patch, if people all agreed the changes 
>> proposed.
> Thanks for supporting this.
>>
>> I pulled your patch and generated the webrev at
>>
>> http://cr.openjdk.java.net/~sherman/7084245/webrev
> I already created a new webrev that handles the suggested changes of 
> Mario Torre.
> They sounded reasonable for me.
>
>>
>> with couple changes from your original patch.
>>
>> (1) Undo the changes in DecimalFormat.java and Format.java.
>>      while arguably your suggested change might be the correct/better 
>> one for
>>      the situation, but it's a behavior change, personally I don't 
>> think you want
>>      to change the behavior, whether it is a "better" solution (not 
>> just look better)
>>      or "bug fix",  in this kind of clean-up project. Leave that to 
>> separate "bug fix"
>>      patch, if you believe the existing code is buggy/wrong.
> Not changing this at all is not the intention of this. Even if the 
> CloneNotSupportedException
> cannot be thrown, chaining is what can be done to make it more clear 
> what happend.
> I don't think that i changed behavoir that much:
>  - Returning null in Format will mostly (if not always) result in 
> NullPointerException in
>    the extending class.
>  - DecimalFormat is the only class in jdk i found that makes it 
> "correct" and catch it and throw
>    an InternalError.
>    If i think about how the catch in DecimalFormat maybe come into the 
> codebase it must
>    be that Format.clone must somehow returned null in the past. Which 
> is a bizarre case,
>    because it cannot happen cause Format is cloneable.
>>
>> (2) Removed your comment and left the printStackTrace() in LoopPipe.java
>>      untouched. Again, that  printStackTrace() might be 
>> unintentional, a leftover of
>>      the debug version for  example, but I'm not sure. Until the 2d 
>> engineer that is
>>      the case, I would prefer to leave it un-touched.
>>
>> (3) Removed the concurrent package changes from the list. As 
>> discussed previously
>>      These changes might go different path to get it, if agreed.
> Done a special webrev for this here:
>     
> http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_concurrent_0/
>>
>> It appears Xuelei.Fan from security might have some options on the 
>> changes made
>> in security area. So Xuelei, do you want me to pull out the changes 
>> in security
>> area and leave that for your to deal with separately?
> Xuelei.Fan gave some input for the security area. He said
> <quote>
>         Exceptions thrown by a method should be defined at an 
> abstraction level
>         consistent with what the method does, not necessarily with the 
> low-level
>         details of how it is implemented.
> <quote/>
> and i think he is totally right. But if i read it right in Throwable 
> this is exactly
> what the Throwable.cause is intended for.
> <quote>
>  * One reason that a throwable may have a cause is that the class that
>  * throws it is built atop a lower layered abstraction, and an 
> operation on
>  * the upper layer fails due to a failure in the lower layer.  It 
> would be bad
>  * design to let the throwable thrown by the lower layer propagate 
> outward, as
>  * it is generally unrelated to the abstraction provided by the upper 
> layer.
>  * Further, doing so would tie the API of the upper layer to the 
> details of
>  * its implementation, assuming the lower layer's exception was a checked
>  * exception.  Throwing a "wrapped exception" (i.e., an exception 
> containing a
>  * cause) allows the upper layer to communicate the details of the 
> failure to
>  * its caller without incurring either of these shortcomings.  It 
> preserves
>  * the flexibility to change the implementation of the upper layer 
> without
>  * changing its API (in particular, the set of exceptions thrown by its
>  * methods).
> <quote/>
>
>>
>> Personally I think it's nice to make the switch to the new chaining 
>> version if the
>> original code is "new InternalError(msg) + initCause()", in which 
>> case it has the
>> clear intention that implementation does want to expose the original 
>> cause of
>> this InternalError. But for the case of InternalError(msg) only, the 
>> proposed change
>> actually again changes the behavior/intention of the original code, 
>> in which it might
>> not be desirable to expose the original cause when throw the 
>> InternalError. As
>> suggested in the Throwable API doc, the "hiding" might be purposely, 
>> to separate
>> different levels of abstraction when throw the InternalError.
> Two comments on this:
> 1.    You maybe haven't noticed it. "new 
> InternalError(msg).initCause()" is handled in a previous patch
>        http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c43af666d130
>
> 2.    You share this opinion with Xuelei.Fan. I think exactly the 
> other way around. That
>        maybe comes from my application development background where 
> intensive logging
>        and long stacktraces are the only information basis you got in 
> production code.
>        The only reason i see to be carefully with long 
> exception-chains is GC and serialization/transfer-costs
>        as i have written in
>        
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-August/007562.html
>        I will not say that Alan is supporting this meaning but he 
> somewhat followed the idea in his follow-up.
>
> I think we should think about it and get some more general policy when 
> to chain and when not and document it
> in Throwable javadoc more clearly. I cannot find the place in 
> Throwable API you mentioned about hiding.
>
>> -Sherman
> -- Sebastian