Fix sm exts reload crashing because it doesn't update native pointers#2418
Fix sm exts reload crashing because it doesn't update native pointers#2418bottiger1 wants to merge 2 commits intoalliedmodders:masterfrom
Conversation
…. Automatically reloads dependent plugins.
|
Do you mind elaborating on the nature of the crash and why this is the best fix? Has it been tested and if so, how? |
This comment was marked as off-topic.
This comment was marked as off-topic.
Kenzzer
left a comment
There was a problem hiding this comment.
I haven't gauged the ramifications of those changes. If I take the PR at face value, and the bug it claims to fix, I think this is a good PR.
I'll provide an in-depth review after @Headline's comment is addressed.
One request change though in the meanwhile.
Before when you reload the extension the pointers to the natives are still pointing to old memory. The only way to cleanly refresh the pointers is to reload the plugins. I do not know how to reload the the pointers in the JIT. I am testing it by reloading an extension I am making so I don't have to do sm plugins unload test;sm exts unload ext;sm exts load ext;sm plugins load test. |
|
Support for unloading was probably one of the worst decisions we made in SourceMod. At the time we were just copying weird behavior in AMX Mod, which reloads plugins on every map change. We dropped that early on because it seemed stupid, but for some reason, never made the rest of the connection. Unloading is very, very complex... as evidenced by how complex this change is. It's difficult to get right, and full of edge cases that no one ever tests... and the vast majority of people don't use this functionality. Most languages and plugin systems do not have any concept of unloading. The only reason why unloading is possible in SourceMod is because of how restrictive SourcePawn is. Did you know that unloading is the biggest thing stopping us from having memory management? This is why we're stuck on "one heap per plugin" which effectively treats each plugin as its own process, forcing slow IPC between them via Push/CallFunction, with no ability to share data structures. So, I would flip question around: why does anyone need this? How much code and complexity can we remove by just dropping unload support? "meta reload sourcemod" should be enough. |
Well I just mentioned why I use it above. Developing or updating an extension without having to reboot the server is very useful. What happens if you discover an exploit or crash and you either have to kick everyone off the server to fix it or wait the whole day until everyone leaves? "meta reload sourcemod" is not viable because reloading everything just to test or update 1 extension is not something people do. Many extensions also have bugs that prevent them from being unloaded without crashing. But some of them don't. I take special care to make sure my extensions without natives are always reloadable.
If C# can do it, why is it not possible in sourcepawn?
I do not see this change as being complex, it just creates 1 new non-virtual method, and the rest of the logic is inside CLocalExtension::Reload, something that already exists. It is self contained except for 1 small method. If the code is bad, it's not going to break anything outside of this. Even if it turns out to have problems, it is still an improvement of the current sm ext reload. Sourcemod has working support for reloading plugins without crashing, why not extend the same courtesy to extensions? |
|
If you don't see this code as complex, something is wrong. It includes like five new data structures. "working" is generous. The plugin implementation relies on authors confirming to a specification that is not easy to test. There are known bugs, that I gave up trying to fix. c# only supports unloading at the assembly load context or app domain boundary, which is similar to our per plugin isolation. |
|
For development iteration, ask Gemini or Claude or something to write a script to automate this. Pushing a new binary and relaunching an application is trivial. Put another way: do you want dynamic strings, arrays, and objects, or do you want to be able to unload plugins? |
|
The data structures are just local sets maps vectors that are thrown away once the function finishes. I have 0 problems with plugin reload, it feels 100% working to me except for extensions not calling OnLibraryAdded which I fixed here. #2417 |
|
I'm fine with this going in, I'll leave that to others. But the consequences of having this functionality are big. If it were me, I would spend time thinking about how to not rely on it, since there might come a time where all plugins share a single heap, and unloading those individually will not be possible. And at that point, it's the same as meta reload. |
|
So you are going to make sm plugins reload/unload stop working as well? |
Except for a few things, I would say the 'now' is more important than the future. If fixing this crash helps sourcemod users in their development cycle, then let them have it. Accepting this PR won't mean you can't backtrack the whole unload/reload/load feature later. |
I don't see this having bigger consequences than removing extension or even plugin unload. Removing the ability to unload anything so there can be garbage collection will break over a decade of expectations. And if you have 1 leaking plugin you're going to have to reboot the server? I would rather have reloading than gc. But I still don't understand why gc is impossible without removing reloads. |
|
While direct extension reloading is undoubtedly convenient for developers, I've always been skeptical about the practical utility and necessity of this feature. Unlike plugins, extension internals are essentially a black box to SourceMod. Consequently, dependencies between extensions and plugins can extend well beyond simple native bindings; for instance, cached memory addresses can easily lead to a crash upon reloading. To completely prevent such crashes, robust hot-reloading logic must be implemented on both the extension and plugin sides—a requirement most authors clearly overlook. Therefore, this feature is only truly viable for developers intimately familiar with both codebases. For the vast majority, if updating an extension in a production environment is necessary, I believe the safest approach will always be to script a full reload of the dependency chain, like so: I've tested this approach and it avoids crashes. You can just run a similar sequence during a map change instead of fully restarting the server. |
This is what my pull request does, so you don't have to do all that yourself. And with sm exts reload you are reloading extensions you know to be safe to reload. For a system based around plugins, not having the option to reload them seems crazy. Reloading is not only valuable to a few developers, it's also good to be able to unload leaking/laggy plugins without having to restart the server. These days, especially in older games, shutting down your server means the server is empty until the next day. If you are doing something insane like shared memory between extension and plugin... then don't reload that extension! Why make it impossible to reload just because some extensions or plugins aren't designed for it? |
I agree your PR is a solid fix for the existing command. However, live-reloading remains a developer tool; safely using it still requires deep knowledge of both codebases. Most server operators can't know if a mid-game reload will cause unexpected behavior or crash the server. Even plugins can easily malfunction if reloaded mid-game due to missed initialization steps. Outside of debugging, server owners actively avoid live reloads. If the goal is to update extensions without restarting the server, SM should handle them like it handles plugins: detect file changes and auto-reload dependent plugins (or all plugins, to safely prevent potential crashes) on map change. That is a much safer and practical approach for everyone. |
Obviously, it's very difficult to change this now, as some large plugin suites have already built their core architecture entirely around dynamic load/unload commands. I think plugin load management commands should have at least been gated behind a toggle in For small plugins, half the code is often just boilerplate to handle reloads safely. For more complex plugins, flawed reload logic is even worse: instead of a clean crash, the plugin usually appears to be running normally but enters a corrupted state—much like an out-of-bounds string—causing completely unpredictable behavior. |
I don't get this thinking. This is already a problem now... all this pull request is make is crash a lot less. How does this pull request make the situation any worse?
It should not, that is way more unsafe. Extensions are way more likely to crash on unload and it should be up to the user to know which ones are safe to reload by trial and error and reload them 1 at a time. By making extensions autoreload on update, you're destroying the usage pattern where you upload an extension update and just wait for the server to reboot when it is empty. Now it will crash on map change instead. |
To be clear, I fully support your PR. It strictly improves the existing command and effectively reduces crashes. My disagreement is solely about encouraging mid-game live-reloads in a production environment. I maintain that it remains a debugging tool. Operators shouldn't rely on this command to update extensions mid-game unless they intimately know the code of both the extension and the dependent plugins. Reloading an unfamiliar extension mid-match is a gamble: you risk immediate crashes, broken gameplay, or—worst of all—silent corruption where the server continues running in a broken state. Even if a live-reload worked previously, it doesn't guarantee safety after future code changes or during different game states. You're just betting against the worst-case scenario, which contradicts the goal of a safe, seamless update.
Why is a map-change reload safer? Because the server state is "clean." Extensions and plugins initialize as if it's a fresh load, mid-execution events aren't missed, and it aligns with how most developers expect their code to run. It avoids mid-game lag spikes and doesn't require the operator to be a programmer to ensure safety. (If there is a strong demand for frequent extension updates, an auto-reload feature could simply be put behind a cfg, letting operators opt-in to that behavior.)
Finally, your point that "extensions are way more likely to crash on unload" perfectly highlights my original skepticism about the reload feature itself (not your PR). Reloading extensions is inherently far more complex and dangerous than reloading plugins. Outside of developers specifically wiring their code for hot-reloading during debugging, I don't believe anyone should be updating extensions without a full server restart. |
I'm not encouraging anything. Trying to discourage people from doing mid-game live reloads by blocking any fixes to reload seems insane to me.
Most plugins can be reloaded just fine. Many extensions can't, but a lot of them can, not including those with natives.
Just because some extensions are coded poorly doesn't mean you need to throw the baby out with the bathwater. It is extremely valuable for the extensions that can be reloaded. |
I am not trying to block this fix or stop anyone from using it. I am simply saying: just because people can live-reload, doesn't mean they should rely on it for production updates. If you haven't run into issues doing this, you've likely been lucky. Perhaps the plugins you run are relatively simple and their authors happened to include flawless hot-reload logic. However, there is also a high probability that your server has entered the "silent corruption" state I mentioned, and it just hasn't been noticeable. AlliedModders does not require published plugins to support mid-game live-reloads, nor do authors guarantee it. Without a doubt, loading updates during a map change—which aligns with the expected lifecycle—is far safer. For complex plugins, even if the author accounted for hot-reloading, loading them mid-match means missing crucial initialization phases. Certain data becomes impossible to retrieve or gets permanently overwritten. I could list plenty of specific examples, but I don't want to derail the discussion. Personally, I struggle to understand the absolute necessity of updating a plugin or extension while players are actively in a match. Since my server hosting experience is primarily with Left 4 Dead 2, I can only guess: does the game you run permanently kick all players during a map change, forcing you to update while they are playing? Sorry, I realize this has strayed from the PR itself, and perhaps different game communities handle these issues very differently. Please don't take it to heart. Regardless, as a developer, I really appreciate your fix for the command. |
Kenzzer
left a comment
There was a problem hiding this comment.
To re-assure @bottiger1 I am keeping an eye on this PR, since there's at least me convinced to merge a fix for this. I dont believe there's anything wrong in bringing parity with plugins reload. If we have to break reload in the future well we will, but in the meantime I don't see any reason not to fix a crash for an helpful dev command, even if in your case its not the intention.
I'll be waiting on headline's thoughts, and of course psychonic's.
Agreed |
Automatically reloads dependent plugins.