<i18n dev> Request for review: 7084245: Update usages of InternalError to use exception chaining

[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