dialogos-project / dialogos

The DialogOS dialog system.
https://www.dialogos.app
GNU General Public License v3.0
21 stars 8 forks source link

plugin settings cross-contaminate across documents. #169

Open timobaumann opened 5 years ago

timobaumann commented 5 years ago

I wonder how to fully resolve the underlying issue in #165 : some plugins keep states (the synthesis plugin has a default speaking tempo, LEGO has devices, SQL has its database) and loading a second document from file later will overwrite the values for all other documents. (I tried this with SQL but would think the same happens for LEGO; synthesis simply forgets its assigned tempo.) It's less concerning that we overwrite what's in memory (i.e., no two ducuments open at the same time that require access to different SQL databases), but it's annoying that they would even overwrite each other's contents if saved. @alexanderkoller: do you think your plugins suffer the same problem and do you have any good/simple+easy-to-implement idea of how to avoid it?

alexanderkoller commented 5 years ago

Oooh, if that's accurate, that would be an absolutely terrible design flaw in DialogOS which we should fix asap.

At first glance, I don't quite understand how this could happen. PluginSettings objects are created in the constructor of com.clt.diamant.SingleDocument; thus, as far as I can tell, each document should have its own instance of the settings of each plugin. Do some plugins - or the code that reads and writes plugins to the XML file - use static variables that are shared across instances of PluginSettings? Or does the Properties mechanism introduce some weirdness?

Can you provide a minimal example (with an existing or a new toy plugin) that illustrates this cross-pollution? I'm probably best positioned to look into this right now, and would be happy to.

timobaumann commented 5 years ago

There's an Interface com.clt.dialogos.plugin.PluginRuntime which may have been part of the solution in the past:

git log tells me that (the current repo) used to contain references to getPluginRuntime() but those seem to have gone away since. @alexanderkoller, can you take a look at older sources and check if you find out what these runtimes were meant to be and how they were meant to be used?

alexanderkoller commented 5 years ago

As far as I can tell, a PluginRuntime is created for each execution of a graph (i.e. too short-lived to be useful here). It is used e.g. in the NXT plugin to hold the connection to the Lego brick.

Subclasses of Node can get access to their PluginRuntime object during execution via Node#getPluginRuntime.

As I said above, the Settings object should have the correct lifespan right now: one instance for each SingleDocument object. Could you give me a minimal example that illustrates the cross-pollution of Settings objects? I'd be happy to look into it.

timobaumann commented 5 years ago

Can you provide a minimal example (with an existing or a new toy plugin) that illustrates this cross-pollution? I'm probably best positioned to look into this right now, and would be happy to.

See 12.zip:

I know that the SphinxPlugin is really not a good candidate for a minimal example but that's all I got for now.

alexanderkoller commented 5 years ago

Thank you! I will look into it later this week.

timobaumann commented 5 years ago

this doesn't seem to be a principled problem with the architecture but only with some fauly plugin implementations. I've fixed SQLite, Sphinx is still broken. MaryTTS and ROS weren't affected.