commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luc Maisonobe <Luc.Maison...@free.fr>
Subject [math] Re: Single root for Exceptions
Date Wed, 29 Aug 2012 08:02:25 GMT
Le 29/08/2012 01:40, Gilles Sadowski a écrit :
> Hi.

Hello,

> 
>>> [...]
>>
>> I think I get your point, but again given transitive / nested
>> dependencies I would not want to depend on it, even if all of the
>> components have single-rooted exception hierarchies.  This is
>> especially true if not all components adhere to the "wrap
>> everything" rule - i.e., if they can generate and/or propagate RTEs
>> that do not inherit from their base exception class.  From the
>> standpoint of the caller, for example, what is the difference
>> between [math]
>>
>> 0)  throwing IAE
>> 1)  throwing MathIAE derived from IAE
>> 2)  throwing MathIAE derived from MathRTE (base)
>> assuming that [math] is not signing up to wrap and rethrow every
>> exception - including IAE - we get from JDK classes?  Will the

I was talking only about what we do throw ourselves.

>> caller actually do anything different if the RTE is math-wrapped vs
>> "naked" but coming out of the [math] code?  I understand that the
>> try/catch may be several layers removed from the code calling a
>> [math] API. 
>>
>> Same applies to NPE, which we don't subclass now, but mostly handle
>> as IAE.
>>
>> I guess one thing we might consider is trying to design for the
>> invariant that we never propagate RTEs without wrapping.  But that
>> would be a lot of work to retrofit and would have a performance cost.
> 
> I don't think that Luc means that we must wrap everyting in a home-made
> exception.

Gilles is right, I did not intend to catch and wrap everything.

> 
> Two possible cases for NPE:
>  * The caller passed a "null" reference and will get, sooner or later, an
>    NPE: it's a bug in his application.
>  * An NPE was raised by a bug in CM, and we must fix it.
> 
> I think that we should not check for "null" and thus have no use for an NPE
> wrapper.

I agree. We shouldn' wrap this.

> 
> Are there other "naked" exceptions that could come out of CM?
> The policy of extensive precondition checking has the purpose (I think) to
> prevent unexpected ("naked" or not) exceptions. If some slip through
> nevertheless, doesn't it mean that we miss some check?

I think we catch what we need to catch and already have a ggod deal of
checking. Of course, we surely missed a few cases, we will fix them when
they are identified.

> 
>>
>>> Another problem is maintenance. Even if you consider the intermediate
>>> developer did his work really accurately and managed to identify all
>>> exceptions thrown by the methods he calls in one version of Apache
>>> Commons Math. When we change an error detection and decide that a method
>>> that did throw only MaxCountExceededException a method should throw
>>> NumberIsToolLargeException instead (or in addition to the existing one),
>>> then the calling code would still compile, but the new exception would
>>> now go all the way upward. The two exceptions have no common ancestor
>>> that can be catched, except Exception itself. With a single rooted
>>> hierarchy, users can use some defensive programming: they can catch the
>>> common root and be safe when we change some internal details.
>>>
>>> A single root would also bring two things I find useful.
>>>
>>> The first useful thing is that the ExceptionContextProvider could be
>>> implemented at the root level, so we could retrieve this context (in
>>> fact, I sometime needs even to retrive the pattern and the arguments
>>> from the context, and we also miss getters for that, but they are easy
>>> to add). It is not possible to catch ExceptionContextProvider because it
>>> is not a throwable (Throwable is a class, not an interface, so we
>>> inherit the Throwable nature from the top level class, not as
>>> implementing the ExceptionContextProvider interface.
>>>
>>> The second useful thing is for [math] development itself. With a single
>>> root, we can temporarily change its parent class from RuntimeException
>>> to Exception, then fix all missing throws declaration and javadoc, then
>>> put the parent class back before committing. This would help having more
>>> up to date declarations. For now, I am sure we have missed a lot of our
>>> own exceptions and let them propagate upward without anybody knowing it.
> 
> "let them propagate upward without anybody knowing it"
> What do you mean? [Of course, all CM exceptions propagate upwards; that's
> the purpose of raising them in the first place.]
> Or did you just mean that the documentation is missing?

I meant the documentation is missing.

> 
>>> As a test, I have just changed the parent for
>>> MathIllegalArgumentException to Exception. I got 1384 compilation
>>> errors. Just going to the first one (a constructor of
>>> BaseAbstractUnivariateIntegrator), I saw we did not advertise the fact
>>> it may throw NumberIsTooSmallException and NotStrictlyPositiveException,
>>> neither in a throws declaration nor in the javadoc. I did not look at
>>> the 1383 other errors...
>>
>> This is a good point.
>>>
>>>> What I am missing is how knowing that an
>>>> aspecific RTE came from within [math] makes a difference.  I am
>>>> skeptical about ever depending on that kind of conclusion because
>>>> dependencies may bring [math] code in at multiple levels.  Also, is
>>>> there an implied assumption in your ideal setup that *no* exceptions
>>>> propagate to [math] clients other than MRTE (i.e. we catch and wrap
>>>> everything)?
>>> No, I don't make this assumption. I consider that at upper levels, code
>>> can receive exception from all layers underneath ([math] at the very
>>> bottom, but also other layers in between). With two or three layers, you
>>> can still handle a few library-wide exceptions (see my example with
>>> MathRuntimeException, and MylibException above). However, if at one
>>> level the development rules state that all exception must be caught and
>>> wrapped (this happens in some critical systems contexts), then a single
>>> root hierarchy helps a lot.
>>
>> But if we allow some exceptions to propagate unwrapped, this does
>> not work, unless I am missing the point here.
> 
> AIUI, when a CM exception is thrown, one (obviously) knows that CM threw it.
> When another exception (not a subclass of "MathRuntimeException") is thrown,
> it did not come from a "throw" statement written in a CM source file.

Right.

> 
>>>
>>> My point is that with a single root, we can get the best of two worlds:
>>> large scope catches and pinpointed catches. The choice remains open for
>>> users. With a multi-rooted hierarchy, we force users to duplicate the
>>> same work for all exceptions we may throw, and we also force them to
>>> recheck everything when we publish a new version, even despite we
>>> ourselves fail to document these exceptions accurately.
>>
>> We need to fix the documentation.  If going back to a single root
>> makes automatic detection of gaps possible, that by itself is almost
>> enough to get me to agree to go back to the single root.  Your
>> arguments above (which I honestly only partially follow) are enough
>> to make me +0 for this change.  I think I probably put too much
>> weight on favoring standard exceptions when we are really only
>> talking about "reinventing" a handful of them.
> 
> I think that there is no relationship between single root hierarchy and
> fixing the documentation...
> [Unless we mean to indiscriminately indicate
> ---
>   @throws MathRuntimeException if something goes wrong.
> ---
> everywhere.]

Single root simplifies this. We hage to apply the trick only once.

Luc

> 
> 
> Gilles
> 
>> [...]
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message