Proposed change to the Interplugin communication API

[worktycho]

The current API for handling inter-plugin communication is simple but broken. Plugins call PluginManager:GetPlugin("Foo") to get a plugin. If the plugin is not loaded it returns nill. There is no way for a plugin to indicate to PluginManager that a plugin is a dependency. If a plugin crashes any plugins that hold a reference to that plugin now hold an invalid pointer with no way to find out that the plugin has crashed. There is also no way to load a plugin after the server has started without modifying settings.ini and reloading all of the plugins complicating experimentation.

Proposal:

Create a new method on PluginManager GetPluginHandle(string)
This method would return a handle which may or may not point to a loaded plugin.
The handle woudl have three hooks OnPluginLoad, OnPluginUnload and OnPluginCrash for handling initialization and destruction of the plugin.
When communicating with the plugin you surround calls with calls to the method TryLoadLock which returns false if the plugin is not loaded and ReleaseLoadLock. It is an error if the plugin is unloaded whilst the lock is in place.

[xoft]

Alright, clarification time.
What do you mean by plugin crashing? There are three behaviors that could be considered a crash:
1, The plugin's code is syntactically incorrect. Lua will complain when the plugin is being loaded and the plugin is never actually loaded. cPluginManager:GetPlugin("foo") returns nil, because there's no such plugin loaded.
2, The plugin's code misbehaves (such as breaking on an assert, calling a non-existent function etc.) while initializing. Again, such plugin will never be loaded, so cPluginManager:GetPlugin("foo") returns nil.
3, The plugin's code misbehaves (such as breaking on an assert, calling a non-existent function etc.) while executing, either a hook callback or a call from another plugin. Lua will unwind the full Lua stack, returning to MCServer; MCServer prints an error message. The plugin is valid and any hook or inter-plugin call can still go through (and possibly succeed).

Now, which ones are solved by using a plugin handle? Obviously #1 and #2 aren't because the plugin won't even load. The plugin handle will make things worse, because it will keep asking MCS to load a plugin that MCS just cannot load.
In #3 the plugin object stays the same and is still usable, so there's really nothing to solve. The only benefit could be the OnPluginCrash; how would that be used by the plugins? Is it a hook, or is it a callback function that the plugin specifies when getting the plugin handle? What if a cascade of crashes happens - plgA calls plgB and plgB calls plgC for some sub-work; plgC crashes and plgB's crash handler crashes too. Ouch!

What I see as a benefit of having a PluginHandle is per-plugin load, unload and reload. Currently if we wanted to implement reloading a single plugin, while keeping the other plugins running, there would be a problem because any of those running plugins may hold a pointer to the plugin being unloaded. That pointer would effectively become invalid and the plugin caught using it will most probably crash the server (if lucky). With a PluginHandle, the situation is somewhat better - the plugin holds a PluginHandle and when reloading the plugin, the internal Plugin object will be swapped inside the PluginHandle. This works nicely, *but*: How about plugin unloading? There's no replacement plugin instance coming into the handle, what now?

Another concern of mine is the need for explicit locking and unlocking. When you force the plugin writers to do something like this, you can be pretty certain that some day someone will forget, or just try what happens if they don't do it. And all hell breaks loose.

[worktycho]

In case 1 and 2 the handle will enter a state indicating that the plugin is invalid and not try to load it. This state will be queryable from the pluginhandle if you want to handle it. In case 3 the plugin is in an invalid state so any use of the plugin will just result in unexpected behavior. It is better to kill the plugin there and reload it. If a plugin with the handle does not hold a loadlock then the OnPluginCrashed callback is called so that the plugin knows that it should cleanup only its own datastructures and not the crashed plugins. If it does hold a loadlock then the plugin with the handle crashes as well as it indecates the plugin is in the middle of something that cannot be interrupted, eg a trady transaction.

For plugin unloading the system will reject the unload command saying that another plugin is holding a reference. As for if a plugin doesnt come up again that is treated as if a plugin failed its initail load.

To prevent plugin authors form forgeting to lock the handle will treat a call without a lock as a crash for early detection. I wish that the system could use RAII locks but they're impossable in lua due to the non-deterministic garbage collector.

[xoft]

Why is #3 an invalid state? Everything works normally, the plugin is capable of running further. It could be for example "cannot connect to database" error; the connection could succeed the next time.

Wouldn't it just be easier to change the plugin calling interface to a single function:

local Ret1, Ret2, Ret3 = cPluginManager:CallPlugin("PluginName", "FunctionName", FunctionArgs);

There's no more cPlugin required, it can even be completely removed from the API. The plugin manager will either have the plugin and call the function, or will not have the plugin and ignore the call altogether. To distinguish the two, you can set up a protocol with the plugin that would require the protocol to always return at least 1 value; if you receive 0 values, then you know the call has failed.

[worktycho]

Because 'Can't connect to the database' should not be an error generating a stacktrace. Errors with stacktraces should be for things where there is no chance of the plugin recovering like invalid iterator or assert failures.
As for youre suggested api, you lose the other major advantages of my API, automatic dependency detection and Transaction safety.

[xoft]

"Cannot connect to DB" is a regular error like any else. Yes, it shouldn't propagate further, but you cannot stop plugin writers from writing such faulty plugins.

I think dependency should be explicitly stated for plugins in their Info.lua file, rather than only inside by the API they use.
And my approach even allows for circular dependencies, because the dependency is specified as late as possible - only when it is actually needed, rather than beforehand.

Consider that there might be multiple "Coiny"-style plugins. Now I want to develop a plugin that can talk to any of those (but don't need all at once). With your style it would mean that there's a hard dependency on each of them; while with my approach the dependency is soft - the one plugin that is called successfully is the one used.

There are multiple levels of dependency in MCS that we should consider:
- "Hard dependency" - the dependent plugin won't work without the dependee. As an example, a chest shop plugin depending on Coiny; without coiny there's no money, so nothing to provide shop value.
- "Soft dependency" - the dependent plugin may provide more functionality if the dependee is present. For example consider ProtectionAreas - WorldEdit interaction. ProtectionAreas does work without WorldEdit, but if WorldEdit is present, you don't want WorldEdit-enabled users to vandalize protected areas.
- "Independent dependency" - There are types of plugins that need to be loaded in a specific order in order to work properly. For example there's a ChatLog plugin and there's ChatColor plugin. Loading ChatColor before ChatLog produces garbage chat-coloring codes in the server console log. The plugins don't have any code-specified dependency, though.
- "Cosmetic dependency" - the webadmin looks the best if the Core is loaded first

Another dependency point that your approach doesn't solve anyway is the order of hooks: If a plugin needs to register its hook before another plugin (like in the ChatLog / ChatColor example above, let's pretend they do specify their dependency in code), the plugin could register its hooks and only then try to load the dependee, which means the hooks will still be registered in the wrong order.

[Taugeshtu]

Why in the love of god would you make us (plugin writers) suffer with thread locks? MCS objects are secured, API is thread-safe (at least proposed to be, and deadlocks are being hunted down), interplugin communication doesn't look to me like it would desperadely need locks.

Also, I don't really see a point in implementing handles. Should you make a handle and implement its states, this will end up with just as good as it would be without handles - with integrity checks before calling a plugin. From plugin's point of view there's no bloody difference between "plugin can't load, never ever forever whatsoever" and "plugin isn't ready for you, try later" - both cases end up in aborting dependant execution. And to be fair, it's not like plugins are "we have internal function that does stuff, but in this plugin stuff's done better, therefore we'll use theirs, but we can use ours as well, it's just not that good"-way. If plugin B needs plugin A, and there's no A to use - B can't work. End of line.

What would really bring good benefit to this is perhaps "OnPluginsLoaded" hook, proposed by xoft somewhere. We had this discussion (on github issues, I believe?), and now I'm convinced that that's where plugin reference getter should go.

Wouldn't it just be easier to change the plugin calling interface to a single function:

local Ret1, Ret2, Ret3 = cPluginManager:CallPlugin("PluginName", "FunctionName", FunctionArgs);

Objection. Should plugin author decide to change plugin's name other plugins (especially if they rely heavily on that one, nd have dozens of calls) would have to rewrite every single call instead of one. IMO, potantial benefit of cleaning API just a bit doesn't justify a potential loss of easy adaptation.

[xoft]

[...] Objection. Should plugin author decide to change plugin's name [...]

If they decide to change the name, they will most likely change other functionality, too, so it makes sense to actually update all the code that interfaces to that plugin. If it hurts so much, you can store the plugin name in a global variable and use that.

The other day I was considering a slight change in how things are - the plugins would have two names, one "internal" exactly for the purposes of interplugin communication and perhaps to identify the plugins if we ever get a "plugin repository"; those names would be restricted in what characters they can use and how long they are etc. Then there'd be "NiceName", a name visible to the server admins, so that the plugin can be called whatever it wants to, and change that name without affecting the functionality. This actually came from the bewilderment about cPlugin:SetName() - what is that function supposed to even do? Why set a name for a plugin that already has a name?

---

[...] What would really bring good benefit to this is perhaps "OnPluginsLoaded" hook [...]

Already implemented: http://mc-server.xoft.cz/LuaAPI/OnPluginsLoaded.html

[worktycho]

Interplugin communication has to be aware of thread locks. Otherwise we need to have a single global lock for lua execution to prevent the A is calling B and B is calling a deadlock. This would cause a number of problems with plugins calling methods that fire hooks. As for the OnPluginsLoaded hook what about if a plugin crash's, You're still left with an invalid reference and there is no threadsafe way of ensuring a live plugin in lua without knowing about locks.

You say that handles are no better than integrity checks, but they have several advantages in allowing the system to handle reloading plugins without having to unload everything, and automatic dependency resolution.

If we go for the OnPluginsLoaded hook, how do you suggest we deal with dependency resolution for destruction order, particularly as destruction can have more than two levels of clean up. The system has no way of telling if plugins have the different destruction dependencies.

If we go for xsofts suggestion and a global variable you've got a hackish version of handles without all the dependency resolution advantages.

[xoft]

I told you already so many times, if a plugin "crashes", the cPlugin object is valid and the server will not crash, no matter what you do with it. You're fixing something that ain't broken.