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

[Xueming Shen]

Sebastian,

src/share/classes/sun/misc/Launcher.java in new patch appears to miss a 
ending } at ln#487

-Sherman

On 08/30/2011 10:20 AM, Xueming Shen wrote:
> Hi Sebastian,
>
> On 08/30/2011 01:23 AM, Sebastian Sickelmann wrote:
>> Sorry i have forgotten the webrev url.
>>
>> http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_main_1/
>>
>>>
>>>> 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.
>>>
>
> We now started to "discuss" whether or not the original code is 
> correct or not, and if not,
> how to fix it. This is exactly the reason why I think this kind of 
> "cleanup" project might not
> want to get involved in, especially if it involves behavior change. 
> Get that done separately
> if you are interested to correct that wrong behavior. In this 
> particular case, the "null return"
> obviously "never" really happens, I agree it's fine to do so.
>
>>>> 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.
> Again, I'm not talking about which approach is correct, which one is 
> wrong. I would leave
> that to the owner of that particular area to decide, whether including 
> the initial cause is
> desired.
> Generally I would assume if it is desired, then they can/should do so 
> with initCause() at
> first place (then, as I said, it's nice to make the switch to the new 
> chaining version, in this
> cleanup project). There are probably three possibilities why they did 
> use initCause() at first
> place (1) the original owner did not think the initial cause should be 
> included (2) even
> it should, since it doesn't matter, they did not include it (3) 
> initCause() didn't even exist
> when the original code was written.  Personally I think you can only 
> do so if it's (2) or (3)
> in a cleanup project. Given initCause() was added in 1.4, I would 
> "assume/guess" most of
> them are because of (3). and actually I believe most of them also 
> belong to "doesn't really
> matter" category.
>
> That said, since 2d/awt, Xuelei.Fan have approved the changes in their 
> areas, and Alan have
> reviewed the changes as well (in which I would assume the core/lib 
> area has been covered).
> I will push the latest changes into the tl repository.
>
> -Sherman
>
>
>