Extensions don't trigger sourcepawn OnLibraryAdded unless you reload them

[bottiger1]

Extensions that call sharesys->RegisterLibrary don't trigger OnLibraryAdded unless they are reloaded.

SourceMod Version Information:
SourceMod Version: 1.12.0.7140
SourcePawn Engine: 1.12.0.7140, interp-x86 (build 1.12.0.7140 NO JIT)
SourcePawn API: v1 = 5, v2 = 16
Compiled on: May 21 2024 12:31:10
Built from: 0f588b87
Build ID: 7140:0f588b87
http://www.sourcemod.net/

Example

public Extension __ext_testlib = 
{
    name = "testlib",
    file = "testlib.ext",
    autoload = 1,
    required = 0,
};

native void TestNative();
public void __ext_testlib_SetNTVOptional()
{
    MarkNativeAsOptional("TestNative");
}

public void OnLibraryAdded(const char[] name)
{
  PrintToServer("OnLibraryAdded %s", name); // doesn't show testlib unless you reload the extension
}

public void OnAllPluginsLoaded()
{
  PrintToServer("loaded: %i", LibraryExists("testlib")); // always works
}

void Sample::SDK_OnAllLoaded()
{
    sharesys->RegisterLibrary(myself, "testlib"); // doesn't matter if you put this here or on SDK_OnLoad
    sharesys->AddNatives(myself, g_natives);
}

[Kenzzer]

Is this a bug report or a feature request ? Is the behaviour you're describing of the forward different if you were to have used a plugin library as dependency instead ?

[MAGNAT2645]

I assume this is a bug report because I also faced this problem. I wanted to use SteamWorks as an optional feature in my plugin so I added #undef REQUIRE_EXTENSIONS before including SteamWorks and added global boolean to track its state (so if it's true I can call its natives). But SteamWorks does not call OnLibraryAdded on first launch.

So in addition to OnLibraryAdded I had to add LibraryExists check in OnAllPluginsLoaded because at this point all extensions are loaded.

[Kenzzer]

Right, but this might just be a load order issue, rather than a bug. If plugin A has X extension as dependency, and plugin B has X extension optional dependency. If plugin A loads before plugin B, then X extension has already been loaded, and OnLibraryAdded will not fire for subsequent loaded plugins. Hence why I'm asking if that behaviour is consistent with plugins that have other plugin library as dependencies.

[MAGNAT2645]

Yeah, but should this happen on first server start (or restart) when all plugins are loading at the same time, not late ones?

[bottiger1]

Right, but this might just be a load order issue, rather than a bug. If plugin A has X extension as dependency, and plugin B has X extension optional dependency. If plugin A loads before plugin B, then X extension has already been loaded, and OnLibraryAdded will not fire for subsequent loaded plugins. Hence why I'm asking if that behaviour is consistent with plugins that have other plugin library as dependencies.

This isn't a load order issue.

OnLibraryAdded is supposed to fire after everything is loaded to send plugins a complete list of libraries. It is not supposed to activate the very instant a library is loaded and give you an incomplete list of libraries based on the random order your plugin was loaded.

sourcemod/core/sourcemod.cpp

Line 510 in 4e73713

scripts->LoadAll(config_path, plugins_path);

sourcemod/core/logic/PluginSys.cpp

Line 858 in 4e73713

LoadAll_SecondPass();

sourcemod/core/logic/PluginSys.cpp

Line 1069 in 4e73713

if (!RunSecondPass(pPlugin)) {

sourcemod/core/logic/PluginSys.cpp

Line 1386 in 4e73713

pPlugin->Call_OnLibraryAdded(lib);

Not to mention that I don't have any other plugin including this extension I tested on, and it doesn't matter if I put RegisterLibrary in SDK_OnLoad instead of SDK_OnAllLoaded. The behavior is the same.

[bottiger1]

I'm guessing you just need to add another line of code under the last one to loop through all existing extensions (m_Lib in the extensionmanager)

Then for each extension you loop through m_Libraries, and call pPlugin->Call_OnLibraryAdded(library);

I have no idea why it works when the extension is reloaded though. I don't really want to spend many more hours staring at the sourcemod internals given that I have a lot of other stuff to do still.

[KyleSanderson]

This is intended, but I do not agree with it. The calling pattern is you call LibraryExists or whatever the native is when your plugin loads.

[bottiger1]

This is intended, but I do not agree with it. The calling pattern is you call LibraryExists or whatever the native is when your plugin loads.

I really don't see how this can be intended. What benefit is there for it to act this way? Why are plugin based libraries handled correctly then but not extensions?

It looks more like an oversight to me where plugin logic is there but extension logic is missing like this #2169

In a correctly working function like LibraryExists, it calls both the plugin system and the extension system.

	if (scripts->LibraryExists(str))
	{
		return 1;
	}

	if (extsys->LibraryExists(str))
	{
		return 1;
	}

So to me it looks very wrong here when only the plugin system is called but not the extension system. Where is the equivalent of ForEachLibrary on ExtensionSys?

		pl->ForEachLibrary([pPlugin] (const char *lib) -> void {
			pPlugin->Call_OnLibraryAdded(lib);
		});

This seems like a relatively easy fix here, but I'm just going to use LibraryExists for now as I still have a lot of other 64bit work to do that takes priority.

[KyleSanderson]

This is intended, but I do not agree with it. The calling pattern is you call LibraryExists or whatever the native is when your plugin loads.

I really don't see how this can be intended. What benefit is there for it to act this way?

I maintain the same comments as you. I had a scenario where I didn't care if the extension was there and didn't need OnPluginStart at all, and ended up writing some fairly invasive macros for it all. This was for an extension that was brought along that materially changed behaviour, and the plugin did not need or want the extension, and has to still modify its behaviour when loaded.

Why are plugin based libraries handled correctly then but not extensions?

You're supposed to set autoload to yes, required to no, then check for it's existence in OnPluginStart. If you don't do this, your extension never loads, or worse, loads late because another plugin correctly identified the load order and you need more boilerplate.

It looks more like an oversight to me where plugin logic is there but extension logic is missing like this #2169

It was intentional, plugin load order is not guaranteed, extension load order (loaded for plugins) is.

[bottiger1]

I am not understanding the conclusions you are potentially making. Are you blocking the pull request based on these statements or are you just pointing out how things are?

If an extension is really optional for your plugin, then you don't even have to declare it as autoload. There's the .autoload file. You could even design sourcepawn logic to load the optimal extension based on which files exist if OnLibraryAdded was fixed.

The fact that you can set autoload=0 required=0 means that there's a case where missing OnLibraryAdded for extensions is broken.

I don't see how fixing this breaks any previous use or unduely negatively affects performance.

[KyleSanderson]

I am not understanding the conclusions you are potentially making. Are you blocking the pull request based on these statements or are you just pointing out how things are?

I was asked privately to provide feedback on the PR, which I am considering now complete. They also did not understand the initial comment.

The fact that you can set autoload=0 required=0 means that there's a case where missing OnLibraryAdded for extensions is broken.

Yes.

I don't see how fixing this breaks any previous use or unduely negatively affects performance.

Again, I agree with you.

[Kenzzer]

Despite also agreeing with the changes, consensus is that extensions are always going to be loaded before plugins. Therefore the usage of LibraryExists is encouraged to figure out whether or not given ext natives are available. The 'late'-fire of OnLibraryAdded for plugin libs is done specifically because plugins load order cannot be guaranteed.

As for autoload=0 required=0 which is a edge case I also raised internally to defend the PR, the consensus again is that this should not be allowed at all. So for this aspect, I'm going to commit changes to evict plugins that compiled against includes with such settings. Better be clear on this.

Additionally (but pending @Headline's review) I'll try to expedite your other PR #2418 since it is loosely related to this matter.

After this, we will have consolidated load order and extensions will always be loaded before plugins. Whether through natural loading or manual reload; thereby ensuring LibraryExists can be relied upon.

[bottiger1]

I understand why things ended up this way now, but this asymmetry doesn't have to continue to exist. I haven't seen any explanations of how this would break behavior that people have relied on, nor any notable performance regression.

If someone is using LibraryExists in OnPluginStart, nothing is going to break if it is triggering OnLibraryAdded now, because they likely don't even have OnLibraryAdded, and if they do it is probably just setting a boolean on a global variable indicating that the extension exists, which is guarded by an if library == "foo".

"extensions are always going to be loaded before plugins" is not true due to people being able to unload and then later load it again.

Telling people to poll LibraryExists for this issue doesn't seem right. The description for the function itself says this:

This function should be considered expensive; it should only be called on plugin to determine availability of resources. Use OnLibraryAdded()/OnLibraryRemoved() to detect changes in libraries.

Why not just fix this? As you pointed out, just remove it later if it comes to that?