Support multiple owners per protocol

[erijo]

Add the missing piece to support multiple owners for protocols.

Each plugin now implements the following two interfaces:

• GeneralPluginFactory or ProtocolPluginFactory (inherits PluginFactory): Used to fetch static data such as name, version, etc. for plugins. It is also used to create and destroy plugin interfaces. When a plugin is first loaded, a factory is created and kept around until the plugin is unloaded.
• GeneralPluginInterface or ProtocolPluginInterface (inherits PluginInterface): This interface is used to control each plugin, e.g. start/stop, send signals, etc.

For general plugins, only one interface is ever created, and this is done when loading the plugin.

For protocol plugins, an initial interface is created when loading the plugin. Then, as new owners are added, new interface instances are created. Each instance is bound to an owner to be able to send signals to the correct receiver.

For access to plugins from other plugins (and also internally in the daemon) we have the classes Plugin and PluginInstance (and their general and protocol variants). The Plugin class maps to a loaded PluginFactory and can be used to query for name, version, running instances etc.

A PluginInstance is used when accessing an instance of PluginInterface. Each PluginInstance runs in a separate thread and has an unique id.

I have enabled multiple owner support for jabber and msn (I haven't tested multiple owner on msn, only jabber). For icq we need to remove the use of statics in the protocol, but also change all callers of PluginManager::getProtocolInstance(LICQ_PPID) to instead get an instance for a specific owner.

[erijo]

Since the diff is huge it is probably easier to look at the individual commits. d494be8 and 7cb33d9 is probably the most interesting.

[flynd]

Good work on finally getting the last big piece done.

I've done some testing with two MSN accounts and it seems to work for the most part. But I found the following glitches:

• Sections in users.conf was written with users from all owners of the same protocol. I've pushed a fix for this.
• Once I got a crash when one of the MSN accounts failed to log in due to invalid password, but I didn't get a backtrace and was unable to reproduce it so I can't say if it's related to multiple owners of if there is some other bug in the MSN protocol.
• When I remove an owner, the protocol instance isn't terminated. Later when I shut down the log shows the instance exiting.

Could you have a look at the last point?

Also, don't forget to update the changelog.

[erijo]

I'll look into the last two items (plus changelog).

One more problem I noticed was if you have the same user on two accounts. Don't really know how we should handle this.

[flynd]

What do you mean you noticed a problem with having the same user on multiple accounts? What kind of problem?

I think I have one or two users in common between the two MSN accounts I tested with and I didn't see any problems. I didn't do any special testing with that though...

[erijo]

The problem I saw was that myUsers list in UserManager is indexed on userId which is the same for two users on different accounts. This causes a memory leak.

[erijo]

The memory leak is in loadUserList. Don't know if there is any other impact than loss of memory for one User, but it should probably be looked into.

[flynd]

UserId contains owner account id to make sure they are always unique (see commit 7998c76d308e51b7f9d378d1beafb8b7d2684967) so this shouldn't be a problem. However there may be some code that I've forgotten to update for this.

Did you actually see a memory leak or was just an assumption?

[erijo]

I only noticed because valgrind told me. I can't say that I looked into it much, but I did confirm that a User instance was lost in loadUserList when adding user to myUsers where the slot already had a User instance. I assumed it wascaused by having the same user in two accounts.

[flynd]

Hmm.. maybe it was related to the bug i fixed in master that made it write all users for the protocol into users.conf instead of just the ones belonging to the current owner. Try cleaning out the bad entries from your users.conf and run valgrind again.

[flynd]

Another thought. Have you thought anything about how to handle account registration? Currently the ICQ registration is broken and disabled, but it might be fixed some day, or another protocol may have a registration feature. At one point I started looking at a Skype plugin that uses dbus to control the Skype application and it would be able to get the owner id automatically after connecting. (Don't get too high hopes on me completing that plugin though, it hasn't happened anything in a long while.) Would it be possible to create protocol instances without a valid owner and let the instance add the owner when registration is complete?

I don't expect this to work right now as there is currently no registration support in the plugin api, but please give it some thought that there aren't any big limitations that would stop this from being possible in the future. As a bonus, it's nice if it wouldn't be limited to a single owner-les instance at a time.

[erijo]

Your fix seems to have fixed the issue with multiple owners having the same user (i.e. the memory leak). I couldn't reproduce the crash with MSN.

As for account registration, I haven't thought much about it. But one way would be to not create an instance for it but instead let the factory deal with it. Or you could create a "dummy" owner that gets removed once the real account has been created. There are probably other options as well, and I don't see any big limitations with this solution for that case. But then again, I'm probably biased ;)

[flynd]

I would guess the MSN crash I saw was not related to these changes.

Registration probably needs to use a real instance in case the protocol is automatically logged in as part of the registration process. Maybe those instances could be handled in a separate list in the plugin manager until they get their owner id. But until we get such a protocol plugin it's not worth solving it.

Another thing though, if I read the code correct you start a protocol instance and call init on it as part of loading the plugin even if there is no owner to create yet. What is the purpose of creating this (dummy) instance? This only seems to make PluginManager::createProtocolOwner longer by having to first check for the dummy instance instead of just always creating one. (Although maybe as a safety check it should have an assert on already having an instance for the same owner.)

I also wonder why the the protocol instances is a list that has to be searched every time an instance is accessed instead of just using a map indexed on owner id?

And I noticed there is a version of getProtocolInstance that takes a protocolId as parameter that is used as getProtocolInstance(LICQ_PPID). I guess that is a quick way to avoid bigger changes to the plugins, but I think most places using it has a variable with the owner that should be used instead of just using the protocol id. In some cases the code only needs the plugin to get the lists of countries/categories/etc. which is static data. That information belongs to the plugin and not to a specific instance so maybe this should be moved from IcqProtocolPlugin to LicqIcq::Factory and let the GUI use getProtocolPlugin(LICQ_PPID) instead for these cases.

[flynd]

I've fixed the places where getProtocolInstance(LICQ_PPID) should be getProtocolInstance(ownerId).

If I move the static ICQ data to LicqIcq::Factory and put it as a separate header in licq/include/licq/icq, can I use plugin_internal_cast for the protocol plugin just like for the instances?

[erijo]

Great work fixing getProtocolInstance. I only did the quick fix to get it compiling :)

For plugin_internal_cast to work on factories you need to add an implementation to plugin.h that would look similar to how it is done in plugininstance.h.

I'll look into the "dummy" instance as well as the list issue.

[flynd]

I'm not sure I follow how everything is working. Could you do the changes needed for casting protocol plugins? Then I can the changes in icq protocol and qt4-gui.

Am I right in thinking your earlier work to separate plugin interface from plugin base classes now only applies to plugin instances and not the plugins themselves any longer?

[erijo]

The plugin interface and the plugin base classes are still separate, but now they are both split in two.

PluginInterface has been split into PluginFactory for the static parts and PluginInterface for the per instance parts.

Plugin has been split into Plugin (maps to a PluginFactory) and PluginInstance (maps to a PluginInterface).

I'm not sure that the names are the best, but that was what I could come up with. If you have any better ideas I'm all ears.

[erijo]

The reason a protocol instance is created when loading the plugin is that plugins are not kept directly in the plugin manager, but in each instance. The idea is that the plugin should only exist as long as we have an instance. Once the last instance is destroyed, the plugin is automatically destroyed.

As for the list vs map question, I think a list is better since it avoid having to store the user id in two places (map and instance).

After 8ec10f3 you should be able to cast a Plugin (using plugin_internal_cast) to any interface (except Licq::PluginFactory) that the plugin's factory implements.

[flynd]

Thanks for doing the cast stuff. I've converted the ICQ data and removed the need for getting an instance just based on protocol id. The instance is still called IcqProtocol which might be a bit wrong, but I couldn't figure out another good name so I didn't think it was worth rename it just for the sake of renaming.

I see your point in closing the library when the last owner is removed, but it adds a (small) quirk to the user interface. When removing an owner, I must re-load the protocol before I can add a new owner even though I never selected to unload the protocol. I still think it's weird to have a dummy instance created just because the protocol is loaded. By keeping a list/map of the protocols as well and just removing the pointer when the last instance is removed you would get the same effect without having the dummy instance. I tried to do this change, but I realized the pluginThread created in loadProtocolPlugin got lost, which makes me wonder if there could be any side effects if the first owner of a protocol is removed so the thread loading and initializing a library is terminated.

I don't see the problem with having the instances in a map. You shouldn't think of it as adding redundant data. It's an index, the whole point is to duplicate data to provide a quicker/easier access. But since the number of entries in this case can be expected to be only a few, the main advantage would be to make the code smaller, which is a good thing. Let the standard classes do the look ups instead of reimplementing stuff.

[erijo]

Good points, I'll look into it once more.

Good work with icq. Not far away from having multiple owners support in icq as well? Only some statics left, or?

[flynd]

I've started looking at removing the ICQ statics a few times, but every time I get lost in the thread handling so for now I think we'll have to do without multiple ICQ owners.

[erijo]

There, now we only create a protocol instance when creating adding an owner.

[erijo]

Any more comments on this or can I merge it to master?

[flynd]

Yes, I think it's ready for merge now.