[ros-users] Draft REP: Automatic unloading of libraries for pluginlib

Dirk Thomas dthomas at willowgarage.com
Mon Feb 20 22:03:08 UTC 2012


 > The only way I see to make this work is to do something like the "weak without shared" hack <http://www.boost.org/doc/libs/1_48_0/libs/smart_ptr/sp_techniques.html#weak_without_shared>. You require
 > the plugin to hold a shared_ptr to itself, with a deleter that only decrements the library reference count. So when Qt deletes the object, the shared_ptr is destroyed and the library ref count
 > decremented. An interesting benefit is that the unloading code could maintain weak_ptr's to the unmanaged instances, to verify that it's actually safe to unload the library.
 >
 > But that's pretty convoluted, and requires the plugin author to inject some messiness into his interface. Maybe it could be wrapped into a reasonable API, but would require some work.

I think we should not require the plugin authors to do something like that or change the current pluginlib API in such a way.


> The real issue here is that Qt releases the object simply by calling delete on the raw pointer. Even if you have a shared_ptr to the object, Qt releasing it does not automatically decrement the
> reference count. So you're actually requiring the user to hang on to the shared_ptr, then explicitly release it when Qt notifies him (by some other mechanism) that the object pointed to by the
> shared_ptr has been deleted. Which is unintuitive, to say the least.

Independently how we design the API the user must take care of the following when using Qt and not regaining ownership of the object before it is deleted:
- call either "delete instance" or being notified when Qt does it
- call either "unloadLibraryForClass()" or drop LibraryHandle as soon as it is no more used

For now we should keep the methods loadLibraryForClass() and unloadLibraryForClass().

I also think that we have a consensus on the following new method:
   boost::shared_ptr<T> createInstance(const std::string& lookup_name)
The usage pattern should be obvious and is described as follows:

   Creates an instance of a desired class (which implicitly calls loadLibraryForClass() to increment the library counter).

   Deleting the instance and calling unloadLibraryForClass() is automatically handled by the shared pointer.


> I agree. Function signatures should be as self-explanatory as possible, because users often won't read the fine print.
>
> I was trying to get at the same point in questioning
>
> T* createUnmanagedInstace(const std::string& lookup_name)
>
> because there's no hint from the signature that this should be paired with a later call to unloadLibraryForClass(). With load/unload, at least it's clear from the names that the methods are paired.

For the "unmanaged" case the user has to take care about the unloading independently how we design the API.
For both cases I would state that the usage pattern is not obvious on a first look to the method signature:
- either the coupling between createUnmanagedInstance() and unloadLibraryForClass()
- or the usage of the LibraryHandle
Since both cases require the user to look into the API doc how the method is meant to be used I would prefer to stick to the simpler signature.

I would propose to keep the following signature:
   T* createUnmanagedInstance(const std::string& lookup_name)

And use the following verbose API doc to hopefully make the usage pattern as clear as possible:

   Creates an instance of a desired class (which implicitly calls loadLibraryForClass() to increment the library counter).

   The ownership is transfered to the caller, which is responsible for deleting the instance and calling unloadLibraryForClass() (in order to decrement the associated library counter and unloading it 
if it is no more used).

Please consider voting on the current implementation.

If you would prefer one of the not implemented variants please mention this in your vote.
I.e.: "-1 as-is, +1 for using the LibraryHandle"

Thank you for your feedback
Dirk



More information about the ros-users mailing list