Better error message in loadNamespace

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Better error message in loadNamespace

Thomas Lin Pedersen
Hi

I’ve just spend a bit of time debugging an error arising in `loadNamespace`. The bottom line is that the `vI` object is assigned within an `if` block but expected to exist for all of the remaining code. In some cases where the package library has been corrupted or when it resides on a network drive with bad connection this can lead to error messages complaining about `vI` object not existing. Debugging through the error is difficult, both because `loadNamespace` is called recursively through the dependency graph and the error can arise at any depth. And because the recursive calls are wrapped in `try` so the code breaks some distance from the point where the error occurred.

I will suggest mitigating this by adding an `else` clause to the `if` block where `vI` gets assigned that warns about potential corruption of the library and names the package that caused the error.

I can open a bug report if you wish, but I would require a bugzilla account for that. Otherwise you’re also welcome to take it from here.

With best wishes
Thomas Lin Pedersen
______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Better error message in loadNamespace

Martin Maechler
>>>>> Thomas Lin Pedersen <[hidden email]>
>>>>>     on Mon, 22 Jan 2018 14:32:27 +0100 writes:

    > Hi I’ve just spend a bit of time debugging an error
    > arising in `loadNamespace`. The bottom line is that the
    > `vI` object is assigned within an `if` block but expected
    > to exist for all of the remaining code. In some cases
    > where the package library has been corrupted or when it
    > resides on a network drive with bad connection this can
    > lead to error messages complaining about `vI` object not
    > existing. Debugging through the error is difficult, both
    > because `loadNamespace` is called recursively through the
    > dependency graph and the error can arise at any depth. And
    > because the recursive calls are wrapped in `try` so the
    > code breaks some distance from the point where the error
    > occurred.

    > I will suggest mitigating this by adding an `else` clause
    > to the `if` block where `vI` gets assigned that warns
    > about potential corruption of the library and names the
    > package that caused the error.

Not sure this is desirable... in general even though it may well
be desirable in your use case...

You will be aware that this an important function that maybe
called many times, e.g., notably even at R startup time and so
must be very robust [hence the many try* settings] and must use
messages/warnings that are suppressable etc etc.

On reading the source, I tend to agree with you that it looks
odd there is no  else  clause to that if(), but then there may
be subtle good reasons for that we don't see now.

    > I can open a bug report if you wish, but I would require a
    > bugzilla account for that. Otherwise you’re also welcome
    > to take it from here.

I'll do that for you in any case.

Martin Maechler
ETH Zurich


    > With best wishes Thomas Lin Pedersen

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Better error message in loadNamespace

Thomas Lin Pedersen


> On 22 Jan 2018, at 16.21, Martin Maechler <[hidden email]> wrote:
>
>>>>>> Thomas Lin Pedersen <[hidden email] <mailto:[hidden email]>>
>>>>>>    on Mon, 22 Jan 2018 14:32:27 +0100 writes:
>
>> Hi I’ve just spend a bit of time debugging an error
>> arising in `loadNamespace`. The bottom line is that the
>> `vI` object is assigned within an `if` block but expected
>> to exist for all of the remaining code. In some cases
>> where the package library has been corrupted or when it
>> resides on a network drive with bad connection this can
>> lead to error messages complaining about `vI` object not
>> existing. Debugging through the error is difficult, both
>> because `loadNamespace` is called recursively through the
>> dependency graph and the error can arise at any depth. And
>> because the recursive calls are wrapped in `try` so the
>> code breaks some distance from the point where the error
>> occurred.
>
>> I will suggest mitigating this by adding an `else` clause
>> to the `if` block where `vI` gets assigned that warns
>> about potential corruption of the library and names the
>> package that caused the error.
>
> Not sure this is desirable... in general even though it may well
> be desirable in your use case...
>
> You will be aware that this an important function that maybe
> called many times, e.g., notably even at R startup time and so
> must be very robust [hence the many try* settings] and must use
> messages/warnings that are suppressable etc etc.

I absolutely agree that even small changes in execution speed
would be bad in such a low-level function. Still, the else clause
would only get called in the event an error should be thrown so I
can’t envision any performance regression.

>
> On reading the source, I tend to agree with you that it looks
> odd there is no  else  clause to that if(), but then there may
> be subtle good reasons for that we don't see now.

If there are reasons for the current construct, then that should of
course be taken into account. As far as I can parse every code
branch that follows the if statement ends up referencing vI, but
lazy evaluation might result in those expression to never be
evaluated so it might be valid calls in some circumstances..?

Anyway, I can accept the argument that changing it might break
things in unexpected ways :-)

>
>> I can open a bug report if you wish, but I would require a
>> bugzilla account for that. Otherwise you’re also welcome
>> to take it from here.
>
> I'll do that for you in any case.

thanks

best
Thomas

>
> Martin Maechler
> ETH Zurich
>
>
>> With best wishes Thomas Lin Pedersen


        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel