Request for review: 7084245: Update usages of InternalError to use exception chaining
sebastian.sickelmann at gmx.de
Tue Aug 30 01:23:55 PDT 2011
Sorry i have forgotten the webrev url.
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
> Thanks for supporting this.
>> I pulled your patch and generated the webrev at
> 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
> 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:
>> 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
> Exceptions thrown by a method should be defined at an
> abstraction level
> consistent with what the method does, not necessarily with the
> details of how it is implemented.
> 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.
> * 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
> * 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
> * the flexibility to change the implementation of the upper layer
> * changing its API (in particular, the set of exceptions thrown by its
> * methods).
>> 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
> 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
> 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.
> -- Sebastian
More information about the core-libs-dev