Fix lua memory leak on reload

[NRH-AA]

Pull Request Prelude

• [x] I have followed proper The Forgotten Server code styling.
• [x] I have read and understood the contribution guidelines before making this PR.
• [x] I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Dereference all lua memory on reload.

Issues addressed: #4941

How to test: Create a global table in a lua script file, fill it with data. /reload scripts Without this change the memory is not released.

[NRH-AA]

Okay, as of now this is working. I had to add reloads because when we init it would dereference everything so we needed to reload the scripts to avoid nils.

Right now everything seems to reload fine. I tested most scripts: spells, weapons, actions, talkactions, ect. And it all works after the reloads. All memory references are removed so the memory when looking in task manager for the server decreases properly. I think this takes care of the lua memory leak. I believe @MillhioreBT is the one that created this for rev's. Is there a problem I am not seeing?

@Shawak @gesior @Codinablack @nekiro @ranisalt

[gesior]

IDK how /reload can be made memory safe (no crashes) and not leaking. /reload was always for dev env, not for production server and it was better to leak, than to crash, but it crashed server in some scenarios anyway. There are many scenarios to test before we merge this:

• mixing scripts and data/lib code
• run other /reload X functions, how they affect each other
• removing events that are bind to some players (ex. onKill)
• what about addEvent functions created before reload?

We got to compare crashes before and after this PR changes.

[ranisalt]

IDK how /reload can be made memory safe (no crashes) and not leaking.

It can't unless we use smart pointers for everything and even then, it may lead to weird behavior - functions that schedule themselves such as periodic events will linger forever

You're right, it should be blocked in production servers

[NRH-AA]

IDK how /reload can be made memory safe (no crashes) and not leaking. /reload was always for dev env, not for production server and it was better to leak, than to crash, but it crashed server in some scenarios anyway. There are many scenarios to test before we merge this:

• mixing scripts and data/lib code
• run other /reload X functions, how they affect each other
• removing events that are bind to some players (ex. onKill)
• what about addEvent functions created before reload?

We got to compare crashes before and after this PR changes.

IDK how /reload can be made memory safe (no crashes) and not leaking.

It can't unless we use smart pointers for everything and even then, it may lead to weird behavior - functions that schedule themselves such as periodic events will linger forever

You're right, it should be blocked in production servers

This all makes sense. I do wonder though, I have tested different scripts quite a bit without crashing. I am not saying it wont crash but the big thing that I noticed is after a /reload over 30mb of memory is released. So why is there so much memory removed without the memory being scripts that aren't loaded?

This probably is a lost cause and /reload shouldn't every be used as you said but there seems to be something here. Perhaps scripts are being loaded multiple times for some reason, idk. I don't have the time atm to really dig into testing/hunting.

[ranisalt]

It's just weird to add these extra function calls, like initState after reinitState. reinitState should probably be modified to do more of what initState does instead, or just dropped and let's use initState only.

Same for reload after clear, reload should probably incorporate clear anyway

[NRH-AA]

@ranisalt @gesior So after doing some more testing what I found is that g_scripts was responsible for the memory leak. We constantly reloaded scripts without ever clearing them out so all the old scripts held onto their old data. (this includes addEvents). I won't be able to do a bunch of testing for now but the code now works as stated before. I would imagine this will take care of any dangling addEvents that caused crashes before.

This is only effecting /reload scripts