views:

125

answers:

3

I have this general problem in design, refactoring or "triage":

I have an existing multi-threaded C++ application which searches for data using a number of plugin libraries. With the current search interface, a given plugin receives a search string and a pointer to a QList object. Running on a different thread, the plugin goes out and searches various data sources (locally and on the web) and adds the objects of interest to the list. When the plugin returns, the main program, still on the separate thread, adds this data to the local data store (with further processing), guarding this insertion point using a mutex. Thus each plugin can return data asynchronously.

The QT-base plugin library is based on message passing. There are a fair number of plugins which are already written and tested for the application and they work fairly well.

I would like to write some more plugins and leverage the existing application.

The problem is that the new plugins will need more information from the application. They will to need intermittent access to the local data store itself as they search. So to get this, they would need direct or indirect access both the hash array storing the data and the mutex which guards multiple access to the store. I assume the access would be encapsulated by adding an extra method in a "catalog" object.

I can see three ways to write these new plugins.

  1. When loading a plugin, pass them a pointer to my "catalog" at the start. This becomes an extra, "invisible" interface for the new plugins. This seems quick, easy, completely wrong according to OO but I can't see what the future problems would be.

  2. Add a method/message to the existing interface so I have a second function which could be called for the new plugin libraries, the message would pass a pointer to the catalog to the plugins. This would be easy for the plugins but it would complicate my main code and seems generally bad.

  3. Redesign the plugin interface. This seems "best" according to OO, could have other added benefits but would require all sorts of rewriting.

So, my questions are

A. Can anyone tell me the concrete dangers of option 1?

B. Is there a known pattern that fits this kind of problem?

Edit1:

A typical function for calling the plugin routines looks like:

elsewhere(spec){
    QList<CatItem> results;
    plugins->getResult(spec, &results);
    use_list(results);
}

...
void PluginHandler::getResults(QString* spec, QList<CatItem>* results)
{
    if (id->count() == 0) return;
    foreach(PluginInfo info, plugins) {
     if (info.loaded)
      info.obj->msg(MSG_GET_RESULTS, (void*) spec, (void*) results);
    }
}

It's a repeated through-out the code. I'd rather extend it than break it.

+1  A: 

Well, option 1 is option 3, in the end. You are redesigning your plugin API to receive extra data from the main app.

It's a simple redesign that, as long as the 'catalog' is well implemented and hide every implementation detail of your hash and mutex backing store, is not bad, and can serve the purpose well enough IMO.

Now if the catalog leaks implementation details then you would better use messages to query the store, receiving responses with the needed data.

Vinko Vrsalovic
Well, the catalog is a global variable in the plugin and the plugin interface doesn't make it obvious you'll be accessing the catalog but besides that...
Joe Soul-bringer
but how does the catalog's interface look like? Is it vector response = catalog.query(params); where query takes care of everything (so plugins do not know there is a hash and a mutex). Or is it acquire(catalog.mutex); response = catalog.hash[key]; release(catalog.mutex); ? If the latter, then don't. If the former, I see no problem. Although it being a global variable smells a bit. Why is it so hard to implement a message that'll query the store and returns the matching data? That is the cleanest option because then there is no option for plugins to mess with the global state.
Vinko Vrsalovic
I haven't written the external catalog interface yet. It wouldn't be too hard to write though. It seems like the first options could just call the second, so you could always do the first option.
Joe Soul-bringer
+1  A: 

Sorry, I just re-read your question 3 times and I think my answer may have been too simple.

Is your "Catalog" an independent object? If not, you could wrap it as it's own object. The Catalog should be completely safe (including threadsafe)--or better yet immutable.

With this done, it would be perfectly valid OO to pass your catalog to the new plugins. If you are worried about passing them through many layers, you can create a factory for the catalog.

Sorry if I'm still misunderstanding something, but I don't see anything wrong with this approach. If your catalog is an object outside your control, however, such as a database object or collection then you really HAVE to encapsulate it in something you can control with a nice, clean interface.

If your Catalog is used by many pieces across your program, you might look at a factory (which, at it's simplest degrades to a Singleton). Using a factory you should be able to summon your Catalog with a Catalog.getType("Clothes"); or whatever. That way you are giving out the same object to everyone who wants one without passing it around.

(this is very similar to a singleton, by the way, but coding it as a factory reminds you that there will almost certainly be more than one--also remember to allow a Catalog.setType("Clothes", ...); for testing.

Bill K
Could you give some more details, I'm sure how this would look in my context (I've pasted a function in)
Joe Soul-bringer
+4  A: 

Why is it "completely wrong according to OO"? If your plugin needs access to that object, and it doesn't violate any abstraction you want to preserve, it is the correct solution.

To me it seems like you blew your abstractions the moment you decided that your plugin needs access to the list itself. You just blew up your entire application's architecture. Are you sure you need access to the actual list itself? Why? What do you need from it? Can that information be provided in a more sensible way? One which doesn't 1) increase contention over a shared resource (and increase the risk of subtle multithreading bugs like race conditins and deadlocks), and 2) doesn't undermine the architecture of the rest of the app (which specifically preserves a separation between the list and its clients, to allow asynchronicity)

If you think it's bad OO, then it is because of what you're fundamentally trying to do (violate the basic architecture of your application), not how you're doing it.

jalf
Your words are strong but correct
Joe Soul-bringer