API review

Proposer: Mirza Shah

Timeline: Please submit feedback by September 5, 2012.

Reviewers:

• William Morris
• Jack O'Quin
• Isaac Saito
• Vincent Rabaud
• Tully Foote
• Ioan Sucan
• William Woodall
• Add your name here when you add comments below.

See the APIReviewProcess for guidelines on doing an API review.

The current implementation can be found at GitHub: ros/plugins.

API Overview

The plugins package is part of an endeavor to refactor the existing plugin system in ROS known as pluginlib. plugins provides a ROS-agnostic way of loading and unloading dynamically-linked libraries during run-time, inspecting what classes (i.e. plugins) are contained within those libraries, and being able to create objects of those classes with only the definition to a base class (i.e. no need for derived class declaration).

Motivation

In its current state, pluginlib has a few shortcomings that we would like to resolve:

1. Dependency on the ROS build system meaning that applications that want to utilize pluginlib but not use the ROS environment cannot do so.
2. Since each pluginlib uses separate named manifests for each class registration the plugin XML also contains this magic name (the name attribute of the class tag). This makes it impossible to use the shared library without knowledge of these magic names and introspect it for available classes.
3. It is not thread-safe

4. It utilizes a hacked version of the third-party Poco library which is undocumented and included as part of the baseline code.

Solution

In order to fix the above mentioned shortcomings, the following has been done:

1. Break up pluginlib into two libraries: plugins and pluginlib. plugins is a new ROS independent package (for which this API review page is being done) for loading/unloading plugins. pluginlib keeps the same interface but sits on top of the new plugins package.

2. Since the new plugins library does not depend on ROS it does not use/require any XML files to work. The library is able to acquire loadable symbols simply by loading a shared library (where as the old pluginlib required the knowledge about the names of the named manifests which is stored in the plugin XML).

   • pluginlib will still utilize the export tags and plugin XML files to discover plugins of specific types in the ROS environment.

3. The new plugins library is designed from the beginning to be thread-safe.

4. The new plugins package utilizes a stock version of Poco directly from the Ubuntu repos.

Solution Design Details

The implemented solution does not break packages that depend on pluginlib as pluginlib's API remains the same. However the implementation of it does.

API

To make a class exportable as a plugin, one must utilize the REGISTER_CLASS macro defined in "plugin_register_macro.h" within their cpp file. For example, if we have a base class called "Animal" and a derived class "Dog" ("Dog" is the plugin), we mark "Dog" as a plugin within the implementation .cpp file as follows: REGISTER_CLASS(Dog, Animal)

Once a class is registered, if it is contained within a runtime library, it will be seen by the plugin system during load time. Note this is an important improvement over the existing pluginlib system where plugin exports have to be explicitly declared within the plugin XML file in addition to utilizing a registration macro.

The API to plugins offers two ways to load plugins:

1. a global function interface (plugins.h)
2. a class-based interface (class_loader.h)

Interface 1 - Global Functions

The first approach to loading plugins is a simple set of functions declared within plugins.h:

template <typename Base> boost::shared_ptr<Base> createInstance(const std::string& derived_class_name);
template <typename Base> std::vector<std::string> getAvailableClasses()
bool isLibraryLoaded(const std::string& library_path);
void loadLibrary(const std::string& library_path);
void unloadLibrary(const std::string& library_path);
void unloadAllLibraries();

Users use the interface as follows:

1. Include "plugins.h" within their source code.

2. Open a library via loadLibrary() passing the name of a runtime library

3. Ask the plugin system which plugins are available for a given base class via getAvailableClasses()

4. Users create instances to plugins via the createInstance() function call

5. Once done, the users close libraries they opened using unloadLibrary() or unloadAllLibraries(). Note if a library is in use by a ClassLoader (next section), then the library won't really be unloaded for real until the last user of the library is finished.

Interface 2 - Class Loader

The second interface is similar, but the functions described above belong to a class known as ClassLoader defined with class_loader.h. Though the global functions in interface 1 are easy to use, sometimes it is desirable to open libraries only within a limited scope. Libraries loaded via a ClassLoader can only use the plugins defined within those libraries if opened through that respective ClassLoader's methods.

class ClassLoader
{
  public:
    template <class Base>
    std::vector<std::string> getAvailableClasses()
    template <class Base>    
    boost::shared_ptr<Base> createInstance(const std::string& derived_class_name)
    bool isLibraryLoaded(const std::string& library_path);
    bool isLibraryLoadedByExternalObject(const std::string& library_path);
    void loadLibrary(const std::string& library_path);
    void unloadLibrary(const std::string& library_path);
};

Users use the ClassLoader as follows:

1. Include the file "class_loader.h" within your source code

2. Create an instance of a ClassLoader

3. Invoke the loadLibrary(), createInstance(), and unloadLibrary() methods in the same way as the global interface

4. Note the ClassLoader will automatically unload libraries in during destruction. Libraries opened by other class loaders or the global interface will stay in memory until the last user is finished.

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

Mirza - I'm fairly happy with how the plugins API came out, though there are some things I'm considering changing/adding.

• The names of some functions/methods/symbols may be tweaked. For example REGISTER_CLASS should be changed to something like PLUGINS_REGISTER_CLASS to prevent name conflicts.

• The library does not generate any error or warning if you shutdown a library while plugin objects exist in memory. A way to solve this is by having the plugin system maintain either a shared or weak pointer to each object generated by it. On library unload, if the shared pointers have a ref count > 1 or equivalently if the weak pointer is still valid, it means there are still objects depending on the library in memory. This is straightforward to implement, but has the expense of a pointer per created plugin.

  • Jack - That sounds reasonable to me.

  • Isaac - It might be more appreciable if the reason why the step of unloading library is necessary is addressed. Honestly I want to know why (with a risk that I might be misunderstanding/missing something though). What I'm used to is: usually I don't have to "close" the libraries that contain classes that are retrieved as shared_ptr (same as the plugins mechanism suggested this time).

    • Mirza - Isaac, great point. Just to make sure I understand you (correct me if I'm wrong), it sounds like you want libraries to automatically unload when objects created from said libraries go out of scope through shared pointers. Likewise we can also have shared libraries load automatically when one attempts to create a plugin object if the corresponding library has not been loaded. This means the user never calls "unloadLibrary" or "loadLibrary" and simply calls "createObject". As plugin objects are destroyed, the last plugin loaded from a library to be destroyed results in the corresponding library to be shutdown automatically. This is a fine design and something we're aiming for at the pluginlib leevl, but it was something we avoided in the low-level plugins library to give control to the user over when libraries are loaded/unloaded. If a user were to keep creating and deleting an object from a single library, given that was the only use of the library, the library would constantly be loaded/unloaded with the user unable to control this behavior. This can have some performance issues as loading a library invokes the OS-level runtime linker and can be a costly operation. However, the more important case is that a user may have multiple versions of the same plugin across multiple libraries. They may want to delete a plugin, unload a library, reload a different library, then recreate an object of the same plugin type. Similarly they may want to unload the library, recompile it, and then reload it. What I'm getting at is that I agree with the automagical approach at the pluginlib level, and that's what we're aiming for...but at the "plugins" level, a finer granularity of control is given.

      • Mirza - An update on this note. Dirk just stopped by and thinks we can make the lower level library more automated, but we're going to slightly change the ClassLoader API again. Standby for an update.

      • Isaac - Thx, the intention is very clear (I just have no idea when I want to use multiple versions of the same plugin during the same execution period, but providing detailed granularity sounds a great idea for the lower level module like plugins).

Bill - I'm really not sure that "plugin exports have to be explicitly declared within the plugin XML file" is a negative. Unless there is some other mechanism, I don't see how the new system works with rospack plugins --attrib=plugin rviz for example. How are plugins in the new system detected?

It is not clear how these changes effect our use cases, but since you are making changes I'd like to propose some ideas.

• As the worlds leading provider of TurtleBot accessories, we have trouble providing all possible URDFs for our customers. To solve this problem, we want to use pluginlib to export URDF fragments for compositing

https://github.com/IHeartRobotics/iheart-ros-pkg/tree/master/ihe_hardware/urdf_compose

manifest.xml

   1 <package>
   2   ....
   3   <export>
   4     <urdf_compose urdf="urdf/cup_holder.urdf"/>
   5   </export>
   6 </package>

• Some hardware we are developing has launch files that should be started at bootup. To make this user friendly we would like to make a launch manager gui, packages could then export launch files that they would like to offer the user to start at bootup

https://github.com/TurtleBot-Mfg/ros-system-tools/blob/master/rosmetalaunch manifest.xml

   1 <package>
   2   ....
   3   <export>
   4     <launch_manager launch="button_interface.launch"/>
   5   </export>
   6 </package>

For some reason it looks like both of these use cases currently work and we would like to maintain and formalize support for this.

Also, this approach may work well for the appmanager to locate .app files provided by various packages.

• Dirk - The whole export infrastructure is not touched by this. So this proposal does not affect your described use cases at all.

Jack - Will nodelets be able to use the new plugins interface?

Jack - How does a plugins host obtain the names of shared libraries it might want to load?

• Dirk - I clarified in the motivation block that the XML infrastructure is not going away. It is still used for the discovery of plugins in the ROS environment. What Mirza wanted to express is that plugins does not rely on these XML information but makes the shared library generally introspectable without further knowledge. pluginlib will sit on top of plugins and utilize the export tags and plugin XML files for discovery as before (just ignoring the name attribute of the class tag).

Jack - I don't understand the purpose of the second, Class Loader, interface. What is the use case? What kind of scope is implied?

• Vincent Rabaud - I am more asking why we have two interfaces. It seems the first one is just the second with a global variable.

  • Mirza - Vincent, the global interface was actually meant to be the original revised interface. The team preferred having the ClassLoader approach instead so as to create different loaded library contexts between objects. In short, the global interface is an artifact of the original design and is not really needed, so I will be removing it.

• Tully: The scoping prevents side effecting when two libraries are looking for the same plugin type. For example if I have two nodelets, A and B. loading libraries One and Two respectively. In nodelet A before nodelet B executes I'd get a list of plugins in library One. In the global case after nodelet B is loaded, the list of plugins available to nodelet A would be the union of plugins in libraries One and Two. Thus nodelet A might only work in the presence of nodelet B side effecting the loaded libraries.

Vincent Rabaud - I'd like to see some const correctness in there: I am not sure about all cases but the following should, imho, be const: getAvailableClasses(), isLibraryLoaded, isLibraryLoadedByExternalObject. I could see why createInstance and the load/unload modify some internal struct.

• Mirza - I agree, I'll attempt to constify the appropriate methods. It may not be an option if the underlying Poco calls aren't const, but I will try.

Vincent Rabaud - I'd like a typedef for the plugin name: this way, when we switch to C++11 and have UTF8 strings, it will be transparent.

• Ioan

Names seem to be consistent elsewhere. Why different function names here?

bool ClassLoader::isLibraryLoadedByExternalObject(const std::string& library_path)
{
  return(plugins::plugins_private::isLibraryLoadedByAnybody(library_path));
}

Can we use self instead of me?

void ClassLoader::unloadAllLibrariesInUseByMe()

• Mirza - Ioan, my bad on the name inconsistency. The API kept changing so somethings didn't get updated. Will fix.

It is nice to call the base constructor here too:

    AbstractMetaObject(const char* name) : name_(name)
    {
    }

and you can inherit privately from boost::noncopyable as well, to avoid having the private constructors, copy , etc. Or better yet, have AbstractMetaObjectBase in herit from boost::noncopyable (privately).

• Mirza - Ioan...I understand, but I don't think it's necessary within the internal implementation and doesn't add much in terms of code bloat. I think boost can sometimes make thinks a little unclear and esoteric...unless the boost::noncopyable concept is used often within ROS in which case I should do it out of convention.

This line should not be in the code:

   std::cout << "MetaObject being destroyed for class type = " << (this->name()) << std::endl;

and all other lines that use std::cout / cerr.

• Take a look at the console_bridge library (on kforge), to see how we get output in a ros independent way from libs, but such that we can connect it to rosconsole in ROS-dependent code. I think this would be a good fit here.

  • Mirza - Ioan, the couts are to be removed. They are there right now for my debugging purposes. However, now that you mention it, I may want to leave some warnings in there so I will utilize your suggestion, thanks!

In plugin_proxy.cpp (which I think it should be renamed to plugin_core.cpp) there are a number of functions that have static members, and each function returns one member. This makes it necessary to write code that does not look very solid (although it might be) where the functions need to be called in a very precise order. Various bells ring for me when I see things like setting the active class loader. I think it would be cleaner if you had a struct like so:

struct PluginCoreStorage
{
 boost::mutex m;
 BaseToFactoryMapMap instance;
 LibraryVector instance;
....
};

and then one single function that does

PluginCoreStorage& getStorage(void)
{
  static PluginCoreStorage storage;
  return storage;
}

• Mirza - Ioan...yeah it's a little messy. Your suggestion helps a bit, but I'm going to leave it for now as I'm rethinking the underlying data structures being used. There are a few things that have to be tracked, the list of open libraries (LibraryVector), the map of base classes to metaobjects (factories), the mutex to protect calls. Within the metaobjects, I'm also tracking the corresponding library and any classloaders that own the metaobjects. The structures I chose are the result of multiple refactorings due to requirements changes and may not be ideal. However, they're reasonable abstractions and I will keep them in there for now until things stabilize and will go back and refactor.

Given the functionality in meta_obejct.cpp, could you use a map instead of a vector for ClassLoaderVector? This is to avoid O(n) searches. This ownership issue however could be resolved with the shared_ptr business I mentioned above.

• Mirza - You're right but I'm going to hold off on this until I'm certain no more things are changing.

What if each loaded library has it's corresponding data in a struct, and there is a shared pointer to that struct in each ClassLoader that needs that lib? The actual unloading happens in the destructor of this struct, so when destroying a class loader, the shared pointer will do the magic (decement ref count and destroy if needed).

• Mirza - Yes, i was thinking of something along these lines, but again, holding off for now.

• Ioan

  • One more thing: plugins seems rather general as a name. Maybe plugin_loader is more suitable?

  • William - I think plugin_loader is too concrete, to me the word plugin implies not only loading a class given a base class but also an interface or context that the class/base class needs to implement. This project simply provides the ability to load a class from a shared object given a base class, it does not imply any plugin like information. Other languages commonly refer to this mechanism as a "class loader" see: http://en.wikipedia.org/wiki/Java_Classloader

    • Ioan I see. How about class_loader then?

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved