<i18n dev> Request for review: 7084245: Update usages of InternalError to use exception chaining
sebastian.sickelmann at gmx.de
Tue Aug 30 01:20:41 PDT 2011
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
I already created a new webrev that handles the suggested changes of
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
I don't think that i changed behavoir that much:
- Returning null in Format will mostly (if not always) result in
the extending class.
- DecimalFormat is the only class in jdk i found that makes it
"correct" and catch it and throw
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
> 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
> 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
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
* design to let the throwable thrown by the lower layer propagate
* 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
* cause) allows the upper layer to communicate the details of the
* 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
> 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
and long stacktraces are the only information basis you got in
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.
More information about the i18n-dev