[ros-users] [ros-pkg] #4887: Extend pluginlib with more info and explicit unloading

[Tully Foote]

On Fri, Oct 7, 2011 at 3:57 PM, Dirk Thomas <mail at dirk-thomas.net> wrote:
>> #4887: Extend pluginlib with more info and explicit unloading
>>
>> Comment(by sglaser):
>>
>>  In this patch, the unloadLibrary calls will silently eat errors.  There
>>  should be a ROS_WARN if the library is not currently loaded, and a
>>  ROS_ERROR if poco fails to unload the library.
>>
>>  The ROS_ERROR is extremely important, because the library cannot be
>>  unloaded if any classes from it are still in use.  Without the error, the
>>  unload may fail, and the user may not realize that the newest version of
>>  his code is not loaded.
>
> Indeed the current implementation of just doing nothing is not good enough.
> I see the following cases:
>
> * calling "unloadLibraryForClass" with an unavailable class:
>  I would propose to throw an exception (introduce LibraryUnloadException)
> which would be similar to what happens for "loadLibraryForClass".
>  Rational: this is definitely wrong usage of pluginlib as the developer
> could verify the availability of the class before.
>
> * calling "unloadLibraryForClass" for a not loaded class (counter doesn't
> exist or is zero):
>  I would propose to also throw a LibraryUnloadException.
>  Rational: this is also wrong usage of pluginlib as it does not follow the
> convention for one unload call for each previous call of load.
>
> Would you agree to implement it that way?

This looks like a fitting implementation.  The exception parallels the
LibraryLoadException, and should inherit similarly form
pluginlib::Exception

>
> Now we get to the nasty cases:
>
> * calling "unloadLibraryForClass" for a class which is loaded multiple times
> (counter > 1):
> * calling "unloadLibraryForClass" for a class which is loaded only once
> (counter == 1):
>  The developer might want to distinguish both cases as in the first case
> reloading the library will NOT load the newest version of the code.
>  I would propose to use the return value for this: false in the first case,
> true for the second.
>
> BUT the developer can never be 100% sure about this.
> Consider the following scenario with two libraries A and B:
> - load A
> - load B (which might unintentionally bind to symbols from A)
> - unload A
> Even after calling "dlclose" (which happens inside of Poco when unloading A)
> the library A will stay in memory due to the symbols still in use.
> Loading A again will therefore NOT load the newest version of the code.
>
> I don't think there is anything we can do about it.
> The developer of the libraries must assure that this cross-library binding
> to symbols does not happen.
> (E.g. by stripping all non-Poco symbols from the library - I will add an
> example for this stripping to the ROS GUI rep soon)
>
> First, did I get it right?
> (This is all written from what I remember from several tests half a year
> ago)
> Do you think this return value would be a reasonable solution?
>

I would probably rather an exception as exceptions are already used
for the other error cases. The developer should already be inside a
try block, and they could then choose either to catch a specific
exception or just warn the user that the unload failed by catching the
generic exception.

TUlly

> Dirk
>
> PS: Another aspect to mention: unloading the library while e.g. instances of
> classes are still in use will very likely result in a segfault.
> As far as I know there is no way to "detect" this beforehand, so ROS_ERROR
> is not an option in this case either.
> _______________________________________________
> ros-users mailing list
> ros-users at code.ros.org
> https://code.ros.org/mailman/listinfo/ros-users
>



-- 
Tully Foote
Systems Engineer
Willow Garage, Inc.
tfoote at willowgarage.com
(650) 475-2827