Clarify the allowed usages of clap_plugin_entry's init() and deinit()

[prokopyl]

This PR is a followup to the discussion in #365, and clarifies the following points in the specification of clap_plugin_entry:

• Assert that a CLAP DSO can be re-initialized after it has been deinitialized.

  The specification currently states that init "can only be called once", and that deinit prevents "[any] more calls to into the DSO". These can be interpreted as "init can only be called once ever" and "nothing in the DSO cannot be used ever again after deinit", which would be an odd requirement, and indeed seems not to be the intent of the spec according to the discussion in #365.

  This PR changes those statements to clearly indicate that re-initialization is allowed, while still forbidding to call init and deinit again if the DSO has already been initialized/de-initialized respectively (which I believe is the intent of the original wording).

• Replace the assertion that confusingly marks init and deinit as "thread-safe", with a more specific explanation of the threading rules for these two functions.

  Using the "thread-safe" term is confusing here because it is usually interpreted as "can be safely called simultaneously from multiple threads", but consecutive calls to these functions are explicitly forbidden, let alone simultaneous ones.

  This is replaced with a more detailed definition, which explicitly forbids simultaneous calls, while also explaining that the host may call init and deinit from any (and different) threads.

  This PR also makes it explicit that those threading requirements do not apply to get_factory, which is thread-safe, and now uses the standard [thread-safe] thread specification.

I also took the liberty to fix a couple of unrelated typos in those specifications. :slightly_smiling_face:

[CLAassistant]

All committers have signed the CLA.

[abique]

Hi,

There is one issue with this specification, which wasn't solved before anyway but it should be addressed if we change this file.

Take the following scenario:

• Host loads plugins A and B
• Plugin A itself hosts plugins and loads B

Here A has no knowledge that the host had loaded B.

Here's one way for the plugin to defensively implement init:

static std::mutex g_plugin_init_mutex;
static int g_init_count = 0;

static bool do_init(const char *plugin_path)
{
   /* actual init function */
}

static bool init_guard(const char *plugin_path)
{
   std::lock_guard<std::mutex> guard(g_plugin_init_mutex);

   ++g_plugin_init_count;
   if (g_plugin_init_count > 1)
      return true;

   if (do_init(plugin_path))
      return true;

  g_plugin_init_count = 0;
  return false;
}

static void do_deinit()
{
   /* actual deinit function */
}

static void deinit_guard()
{
   std::lock_guard<std::mutex> guard(g_plugin_init_mutex);

   if (g_plugin_init_count == 0)
     return;

   --g_plugin_init_count;
   if (g_plugin_init_count == 0)
      do_deinit();
}

I think the correct spec would be:

• thread safe
• init counter
• if init() did return true, then there must be a corresponding call to deinit()

[prokopyl]

That's a good point. I think it's a good idea for plugins to implement init defensively this way (it seems Yabridge does something similar already). We could perhaps add that as a recommendation in the specification.

However, I don't think we can allow multiple subsequent calls to init now (by forcing plugins to use an init counter as you suggested, or any other defense mechanism), as this would be a breaking change to the spec. In this case it could potentially lead to crashes if a host wanted to leverage this new property on a plugin that hasn't been updated.

Moreover, I don't think this solution would help much in the scenario you described, in practice. There are plugins out there that aren't that defensive in their init implementations (and correctly so, since they relied on the spec that allows them not to be), which means that "sub-hosts", like Plugin A in your example, cannot rely on a plugin-side implementation anyway.

As of now, the only solution for "sub-hosts" like this (that I can think of, at least :slightly_smiling_face:), is to spawn their own child process (like Yabridge does) to have their own address space for their plugins.

Alternatively, since the root of the issue here is that "A has no knowledge that the host had (un)loaded B", we could perhaps come up with an extension instead, that would allow for the parent host and the sub-host (A) to synchronize their init counters for a specific plugin's DSO, so that a sub-host doesn't double-load (or unload) a plugin that the parent host is using, and vice-versa. I have some ideas, but of course that would be outside the scope of this PR. :slightly_smiling_face:

[abique]

However, I don't think we can allow multiple subsequent calls to init now (by forcing plugins to use an init counter as you suggested, or any other defense mechanism), as this would be a breaking change to the spec. In this case it could potentially lead to crashes if a host wanted to leverage this new property on a plugin that hasn't been updated.

We may allow it or not, it may still happen.

Moreover, I don't think this solution would help much in the scenario you described, in practice. There are plugins out there that aren't that defensive in their init implementations (and correctly so, since they relied on the spec that allows them not to be), which means that "sub-hosts", like Plugin A in your example, cannot rely on a plugin-side implementation anyway.

It is fine to not be defensive, if you enjoy customer support :)

As of now, the only solution for "sub-hosts" like this (that I can think of, at least 🙂), is to spawn their own child process (like Yabridge does) to have their own address space for their plugins.

Indeed, but that's quite a requirement versus make your init() function robust.

Alternatively, since the root of the issue here is that "A has no knowledge that the host had (un)loaded B", we could perhaps come up with an extension instead, that would allow for the parent host and the sub-host (A) to synchronize their init counters for a specific plugin's DSO, so that a sub-host doesn't double-load (or unload) a plugin that the parent host is using, and vice-versa. I have some ideas, but of course that would be outside the scope of this PR. 🙂

Still, it doesn't work.

You have a vst2 host, hosting plugin A (vst2) and B (vst2), and both A and B open a plugin C (CLAP).

You may also have a plugin that is symlinked into the vst2 and clap plugin folder, then dlopen() will open the same file, so you may end up with the dual init() anyway.

I think the only solution, even today is to have a defensive implementation of the init/deinit thing.

[baconpaul]

So all of my init methods are return true but I agree with Alex, there is no way for a host to guarantee the spec requirement that init is only called once per dlopen and as such if you do something non-trivial you have to defend. In the very real case, for instance, of running the clap and the vst3 wrapped clap in the same process space, you get a double init and (in some cases) double dlopen.

I wish we had realized this earlier of course, but I think we have to at least share that defacto reality in the spec. inits and de-inits match and can be called from any thread is, I think, the current state of affairs of what actually happens to a plugin, independent of the spec.

Whether we update the documentation to say that is an interesting question about how we manage changes in the spec.

[abique]

We could make the spec say that any plugin build with clap >= 1.11.0 should implement the counter behavior.

[prokopyl]

However, I don't think we can allow multiple subsequent calls to init now (by forcing plugins to use an init counter as you suggested, or any other defense mechanism), as this would be a breaking change to the spec. In this case it could potentially lead to crashes if a host wanted to leverage this new property on a plugin that hasn't been updated.

We may allow it or not, it may still happen.

That's true, my concern is that it may happen more if we allow it, since changing it that way allows hosts to be less defensive on their end. As of now, hosts that call init multiple times themselves (before a deinit) are non-compliant to the spec (actually been there, done that recently :sweat_smile:), which helps reduce the likelihood of this problem occurring.

Moreover, I don't think this solution would help much in the scenario you described, in practice. There are plugins out there that aren't that defensive in their init implementations (and correctly so, since they relied on the spec that allows them not to be), which means that "sub-hosts", like Plugin A in your example, cannot rely on a plugin-side implementation anyway.

It is fine to not be defensive, if you enjoy customer support :)

True haha. Here I was thinking about what happens if Plugin B isn't updated anymore and can't catch up to the new spec. You won't get more customer support issues if you don't support customers at all in the first place. :wink:

(Plugin A will likely get a bunch of compat issues for plugin B though.)

As of now, the only solution for "sub-hosts" like this (that I can think of, at least 🙂), is to spawn their own child process (like Yabridge does) to have their own address space for their plugins.

Indeed, but that's quite a requirement versus make your init() function robust.

I don't see how making your own init() function would help here? As I understand it, every plugin out there would need to be patched to make their init() function robust to properly support this use case. If I was Plugin A's author, I'd much rather be more defensive and implement the child process solution, rather than trying to hunt down all of the plugins that aren't defensive enough out there.

Alternatively, since the root of the issue here is that "A has no knowledge that the host had (un)loaded B", we could perhaps come up with an extension instead, that would allow for the parent host and the sub-host (A) to synchronize their init counters for a specific plugin's DSO, so that a sub-host doesn't double-load (or unload) a plugin that the parent host is using, and vice-versa. I have some ideas, but of course that would be outside the scope of this PR. 🙂

Still, it doesn't work.

You have a vst2 host, hosting plugin A (vst2) and B (vst2), and both A and B open a plugin C (CLAP).

Ah, right, I didn't think about that. I use so little of these MetaPlugin-like wrappers, I didn't even think one could have two of them! :sweat_smile:

I also forgot about the VST -> CLAP wrappers, yeah I don't see how hosts could possibly defend against double-init in those cases.

I think the only solution, even today is to have a defensive implementation of the init/deinit thing.

Yeah, I agree. We could heavily suggest that plugins should be defensive in their init/deinit implementations, while still forbidding hosts from do calling init twice directly like the spec does now. I can make that addition in this PR. :slightly_smiling_face:

[prokopyl]

We could make the spec say that any plugin build with clap >= 1.11.0 should implement the counter behavior.

Well I should check for new replies before clicking Send lol. Disregard my previous message, I'll just do that. :smile:

[baconpaul]

I think we should make it say

"Prior to clap 1.11 our documentation stated that init was called once, and deinit once. Unfortunately in some circumstances it is impossible for a host to realize this, thus with clap 1.11 we are simply making the statement that init calls and deinit calls must match. This means the de-facto practice of protecting your init with a locked counter is now the to-spec practice also."

or some such. See what I mean? Don't make it sound like you need "if version < 1.11 then don't lock and count else lock and count" in the docs

[prokopyl]

Yes I see what you mean. I think the best way to do this would be to assert that plugins should implement the locked counter defense strategy (since the issue affects all CLAP versions anyway), and that this becomes a hard requirement as of CLAP v1.11.

While adding a hard requirement is still a breaking change technically, it doesn't break more compared to the way it is done now, so that works for me. ^^

[baconpaul]

yup that's the spirit of what i was getting at too. Agree.

[prokopyl]

I've added that in my latest commit. It's a quite a lot, but I think I've got all of the details and possible side-effects of the issue covered now. :slightly_smiling_face:

[abique]

Hi,

Thank you very much @prokopyl .

I went through it, I think the text is long for my taste, but the history told us that long is better than too short. @baconpaul have you gone through it too? What do you think?

I think it looks good for the merge, we'll still have the possibility to ajust it in the final 1.11.0 review.

[abique]

My style would be more condensed:

1. everything is thread safe
2. init counter
3. note for plugins with clap <1.11.0 which didn't have the init counter requirement
4. Rationale explanation of the design

My issue with a documentation that is too long is that devs don't read it, and when they reach the bottom of it, they forgot what was in the top, so it requires multiple reads.

With that plan you'd get the essential information immediately, and then it is mostly the rationale that takes more line (which you can skip), and the brain already has the big picture when reading it.

[baconpaul]

I have some time tomorrow and will peek. I agree it should be as short as it can while being correct, but no shorter :)

[abique]

I've implemented the entry init counter in the plugin template. I've also implemented it for clap-plugins.

[prokopyl]

Yeah I feel it's very wordy too, while writing it I didn't find any way to condense it while still covering every edge case and the rationale at the same time. There's indeed the issue with it being too long making the brain tune out and miss things, but on the other hand if it's too short you get annoying devs like me who aren't sure what's valid or not and make lengthy discussion posts and PRs, so I guess it's a tradeoff. :stuck_out_tongue:

But your idea of separating the spec part and the rationale is a good idea I think, I'll try that. :slightly_smiling_face:

I've implemented the entry init counter in the plugin template. I've also implemented it for clap-plugins.

Neat! I think it could also be worth adding a check for it in clap-validator, I could make a PR for that once I'm done here. :slightly_smiling_face:

[baconpaul]

Took a shot at making the introductory text briefer.

 // The intent of the init() and deinit() functions is to provide a "normal" initialization patterh
 // which occurs when the shared object is loaded or unloaded. As such, hosts will call each once and
 // in matched pairs. In clap specifications prior to 1.1.11, this single-call was documented as a
 // requirement.
 //
 // We realized, though, that this is not a requirement hosts can meet. If hosts load a plugin
 // which itself wraps another CLAP for instance, while also loading that same clap in its memory
 // space, both the host and the wrapper will call init() and deinit() and have no means to communicate
 // the state.
 //
 // With clap 1.1.11 and beyond we are changing the spec to indicate that a host should make an
 // absolute best effort to call init() and deinit() once, and always in matched pairs (for every
 // init() which returns true, one deinit() should be called). 
 //
 // This takes the de-facto burden on plugin writers to deal with multiple calls into a hard requirement.
 //
 // Most init() / deinit() pairs we have seen are the relatively trivial {return true;} and {}. But
 // if your init() function does non-trivial one time work, the plugin author must maintain a counter
 // and must manage a mutex lock. The most obvious implementation will maintain a static counter and a
 // global mutex, increment the counter on each init, decrement it on each deinit, and only undertake
 // the init or deinit action when the counter is zero.

wdyt?

[abique]

@baconpaul I like it as part of the rationale; but I miss the short, raw and in your face implementation instructions for the developer who's short on time and isn't interested about why the current design is what it is.

[baconpaul]

Right. I agree

so let’s have a tldr and move that text to later in the file

thentldr is

“init and deinit in most cases are called once, in a matched pair, when the dso is loaded / unloaded. In some rare situations it may be called multiple times in a process, so the functions must be defensive, mutex locking and counting calls if undertaking non trivial non idempotent actions.”

[abique]

Sorry, I forgot about this PR. I review it now.

[abique]

I've merged it but I wish @prokopyl did the documentation update that was suggested in the discussion.

[prokopyl]

My apologies for the delay. I know it's quite a bit late now that 1.2.0 is out, but I can still make another PR to incorporate the suggestions if you wish.