Left4Dhooks and OnPluginEnd()

[keyCat]

OnPluginEnd() is not called for any plugin that was dependent on left4dhooks if the library was unloaded before the plugin. This is a major issue, because implementation of each plugin becomes dependent on third-party library ability to reset cvars, free handles, unpatch addresses and so on.

This was never an issue with left4downtown, because the lifecycle of a plugin wasn't affected by an extension (it was never unloaded).

As far as I can tell, your solution to this issue is this plugin: https://github.com/SirPlease/L4D2-Competitive-Rework/blob/master/addons/sourcemod/scripting/indep_unloader.sp

I don't see this as a way forward, because there are more to any server than ZoneMode/NextMod config and we can't add every single plugin clean-up to another plugin.

On the side-note, I'd like to say that I find the idea of moving stocks inside of a plugin (l4d2_direct -> left4dhooks) quite preposterous, harmful and useless. This should have never been done, because this brings to life issues like this one, nevermind the include file growth and complexity.

[SirPlease]

OnPluginEnd() is not called for any plugin that was dependent on left4dhooks if the library was unloaded before the plugin. This is a major issue, because implementation of each plugin becomes dependent on third-party library ability to reset cvars, free handles, unpatch addresses and so on.

I concur, a possible solution for this would be to create a forward that indicates the plugin is about to get unloaded. We could then use this forward in plugins that do make use of Left4DHooks and do stuff on Plugin End.

On the side-note, I'd like to say that I find the idea of moving stocks inside of a plugin (l4d2_direct -> left4dhooks) quite preposterous, harmful and useless. This should have never been done, because this brings to life issues like this one, nevermind the include file growth and complexity.

l4d2_direct is still supported, if it's a big deal you can use the old include as it's a 1:1 copy if I'm not mistaken. I wouldn't call it harmful, while I agree and I'm not a fan of the dependency, there's always ways to work around it. We're still working with confogl after all these years after all, we all know what a mess that is. We'll make it work 👍

[devilesk]

If you change the plugin to make the left4dhooks dependency optional, then OnPluginEnd will still be called in the plugin, even if left4dhooks is unloaded first.

#undef REQUIRE_PLUGIN
#include <left4dhooks>

[SirPlease]

If you change the plugin to make the left4dhooks dependency optional, then OnPluginEnd will still be called in the plugin, even if left4dhooks is unloaded first.

#undef REQUIRE_PLUGIN
#include <left4dhooks>

Mhm, just never really used this in practice so kind of worried about possible error logs coming from this. I suppose it's the "cleaner" option plugin wise though. 👍

[devilesk]

Here's a list of plugin sources that have an OnPluginEnd function and include left4dhooks and don't compile without it:

• l4d2_hybrid_scoremod_zone.sp
• l4d2_weapon_attributes.sp
• holdout_bonus.sp
• readyup.sp
• l4d2_scoremod.sp
• l4d2_hybrid_scoremod.sp
• confoglcompmod.sp

Here's a list of plugins that have an OnPluginEnd function and include left4dhooks but can compile with the left4dhooks dependency removed:

• l4d2_penalty_bonus.sp
• boomer_horde_equalizer.sp
• l4d_mapbased_cvars.sp
• l4d2_tank_support.sp
• 1v1_skeetstats.sp

l4d2lib.smx has a dependency on left4dhooks that comes from mapinfo.inc, but it looks like left4dhooks can be removed from mapinfo.inc and l4d2lib still compiles.

[devilesk]

If you change the plugin to make the left4dhooks dependency optional, then OnPluginEnd will still be called in the plugin, even if left4dhooks is unloaded first.

#undef REQUIRE_PLUGIN
#include <left4dhooks>

Mhm, just never really used this in practice so kind of worried about possible error logs coming from this. I suppose it's the "cleaner" option plugin wise though. 👍

Since left4dhooks is registered as a library, I think the proper way to do it would be to have the plugins use OnLibraryRemoved to check if left4dhooks is removed and if it is then execute the plugins' cleanup logic. I think that's the cleanest way to do it and avoids the question of possible error logs coming from making a technically required dependency into an optional dependency.

[keyCat]

l4d2_direct is still supported, if it's a big deal you can use the old include as it's a 1:1 copy if I'm not mistaken. I wouldn't call it harmful, while I agree and I'm not a fan of the dependency, there's always ways to work around it. We're still working with confogl after all these years after all, we all know what a mess that is. We'll make it work

I called it "harmful", because it makes compile-time dependency into a run-time dependency without any good reason. Sure, we could use the same old l4d2_direct, but there's sort of no point in all that duplication?.. And there's also soft cross-dependency between left4dhooks and l4d2_direct now. They has to mirror each other. Anyway, that's mostly a rhetorical issue rather than practical (for now).

I concur, a possible solution for this would be to create a forward that indicates the plugin is about to get unloaded. We could then use this forward in plugins that do make use of Left4DHooks and do stuff on Plugin End.

If you change the plugin to make the left4dhooks dependency optional, then OnPluginEnd will still be called in the plugin, even if left4dhooks is unloaded first.

Since left4dhooks is registered as a library, I think the proper way to do it would be to have the plugins use OnLibraryRemoved to check if left4dhooks is removed and if it is then execute the plugins' cleanup logic. I think that's the cleanest way to do it and avoids the question of possible error logs coming from making a technically required dependency into an optional dependency.

Yeah, we thought of that, too, but it feels wrong. As if we creating another God-entity (hello to aforementioned Confogl) to dance around. Either way, it's not going to be pretty. Plugin developers that rely on left4dhooks must intuit that they need this special ceremony in order to make it work properly. Dunno, just leaves a bitter taste in my mouth...

[devilesk]

Yeah, we thought of that, too, but it feels wrong. As if we creating another God-entity (hello to aforementioned Confogl) to dance around. Either way, it's not going to be pretty. Plugin developers that rely on left4dhooks must intuit that they need this special ceremony in order to make it work properly. Dunno, just leaves a bitter taste in my mouth...

I agree it's unintuitive, but we should just inform developers on how to deal with it. Using libraries is something they should learn about anyway. It's just not clear that the transition from l4d2_direct/left4downtown to left4dhooks is one from include/extension to library. The README here should be updated with a note about how to properly update plugins from l4d2_direct/left4down to left4dhooks. Explain to developers how they should now consider OnLibraryRemoved if they use OnPluginEnd in their plugin. The first post on the AM thread should also be updated to mention something about this.

If we really want to hide this issue from developers, then I think the best solution would be to update how confogl unloads plugins. Instead of sm plugins unload_all, unload all the plugins in optional/ and other plugin subfolders first, before unloading everything else. That would make sure left4dhooks is always unloaded at the end. SM-PluginMangler looks like it provides this functionality, but I haven't used it.

[SirPlease]

Plugin developers that rely on left4dhooks must intuit that they need this special ceremony in order to make it work properly. Dunno, just leaves a bitter taste in my mouth...

I agree it's unintuitive, but we should just inform developers on how to deal with it. Using libraries is something they should learn about anyway. It's just not clear that the transition from l4d2_direct/left4downtown to left4dhooks is one from include/extension to library. The README here should be updated with a note about how to properly update plugins from l4d2_direct/left4down to left4dhooks. Explain to developers how they should now consider OnLibraryRemoved if they use OnPluginEnd in their plugin.

While I'm obviously and clearly targetting competitive server owners and plugin devs that want to expand on this repo, I do think certain things are outside of the scope of what this repo is for. Either way, I will look into something that's suitable for both parties without too much modification, but it will not be in my High Priority list.

Hopefully that's understandable and fair.

[devilesk]

I concur, a possible solution for this would be to create a forward that indicates the plugin is about to get unloaded. We could then use this forward in plugins that do make use of Left4DHooks and do stuff on Plugin End.

Just in case you were going to implement this, I just saw that confogl already has such a forward called LGO_OnMatchModeUnloaded which it calls before it unloads all plugins.

[keyCat]

If we really want to hide this issue from developers, then I think the best solution would be to update how confogl unloads plugins. Instead of sm plugins unload_all, unload all the plugins in optional/ and other plugin subfolders first, before unloading everything else. That would make sure left4dhooks is always unloaded at the end. SM-PluginMangler looks like it provides this functionality, but I haven't used it.

I decided to entertain this idea...

SM-PluginMangler does not do what we were discussing, so I come up with my own solution. It works, but let's say it's... less than ideal. Not only you would have to replace each sm plugins * call in every config, but it brings its own complexity: while usually CMDs execute from top to bottom in the same frame during the config execution, this is not the case with ServerCommand, so I had to add some frame scheduling, which means that other CMDs used in the cfg might lead to unexpected results due to messed up ordering.

Or maybe, I just chose the wrong approach to implement this. Kind of looks brute-forced.

[SirPlease]

I was/am actually working on a similar solution, but I feel like you went far too deep with your approach. I'm in the process of testing mine and so far it looks like it's working out swell, the only things I have left to do is check out the dependencies and Server command order.

[keyCat]

Yeah, I have the same feeling. Hope your solution works well 👍

[SirPlease]

Looks like it's working smoothly, all that's required to edit is confogl_off for matchmodes. I've added a cvar rather than hardforcing replicating confogl_off's behaviour when it comes to refreshing plugins.

The idea is to put the plugin in optional, load it through generalfixes or whatever shared plugin file you use, after that we modify confogl_off to include:

• confogl_reserve_plugin left4dhooks.smx
• confogl_reserve_plugin optional/readyup.smx
• confogl_unload_plugins optional
• confogl_unload_plugins anticheat
• confogl_unload_plugins fixes
• confogl_unload_plugins
• confogl_unload_reserved

Before I push it though, I'd like you guys to review the code. Perhaps you have suggestions or notice something that I looked over, I'll be away for a bit so when I come back I can start working on possible things I overlooked.

[keyCat]

If I am not mistaken, you could try even simpler approach; store plugins into an array and unload them in reverse order. This should not produce any errors, and also will properly call OnPluginEnd() forwards. At least, this is how it appears in my tests:

ArrayList g_alPluginUnloadQueue;

public void OnPluginStart() {
    g_alPluginUnloadQueue = new ArrayList(PLATFORM_MAX_PATH);

    RegAdminCmd("sm_plugins_unload_all",   Cmd_UnloadAll,  ADMFLAG_RCON);
}

public void OnPluginStart() {
    delete g_alPluginUnloadQueue;
}

public Action Cmd_UnloadAll(int iClient, int iArgs) {
    char szFilename[PLATFORM_MAX_PATH];
    Handle hPlugin = INVALID_HANDLE;
    Handle hIterator = GetPluginIterator();

    while (MorePlugins(hIterator)) {
        hPlugin = ReadPlugin(hIterator);
        GetPluginFilename(hPlugin, szFilename, sizeof(szFilename));
        // just to make sure we unload ourselves last
        if (StrEqual(szFilename, "optional/confogl_unloader.smx", false)) continue;
        g_alPluginUnloadQueue.PushString(szFilename);
    }
    delete hIterator;
    ExecuteUnload();

    return Plugin_Handled;
}

void ExecuteUnload() {
    char szFilename[PLATFORM_MAX_PATH];
    for (int i = g_alPluginUnloadQueue.Length - 1; i >= 0; i--) {
        g_alPluginUnloadQueue.GetString(i, szFilename, sizeof(szFilename));
        ServerCommand("sm plugins unload %s", szFilename);
    }
    g_alPluginUnloadQueue.Clear();

    ServerCommand("sm plugins unload optional/confogl_unloader.smx");
}

(sorry for illustrating it with my code, I'm lazy)

The reason I didn't stop here with my version is because I also have to manage configs and plugins for Vanilla gamemodes when Confogl match is unloaded, which isn't really a concern here 😬