[Master] Hotswap reload on debian

[DoctorKraft]

Description:

I have a problem with hotswap system on debian, when i modify a file (spellscript for example). The recompiler run no problem, but after he doesn't reload lib with modification. I need to restart my worldserver for get modifications.

Steps to reproduce the problem:

1. run cmake with -DWITH_DYNAMIC_LINKING=ON and -DSCRIPTS="minimal-dynamic"
   (also trying with dynamic)
2. Modify a spellscript in Spells folder for example
3. Recompiler run but doesn't load new lib (no change on spell)
4. Restart your worldserver, modification appear

Branch(es): Master

TC rev. hash/commit: 19a0e05

Operating system: Debian 8 Jessie

[Naios]

Is this an issue specific to debian or is there an issue on windows as well?

[DoctorKraft]

Idk i don't have trying on windows :/
Hotswap system use boost lib ?

[Naios]

Yes it does use boost:

• iostreams
• filesystem
• and others

[svranesevic]

How are you starting worldserver, './worldserver' from 'install_path/bin' directory, or ?

[DoctorKraft]

@svranesevic Directly in folder bin ./worldserver

[svranesevic]

Alright, I had similar issue on OS X with #19365, problem was GetDirectoryOfExecutable() in ScriptReloadMgr.cpp not evaluating executable parent directory correctly - Parent path of executable was evaluated in non canonical form (Containing './', or '../', which rendered file watcher unable to watch scripts shared libraries)

Anyhow, can you try this fix on ScriptReloadMgr.cpp ?

@@ -112,7 +117,7 @@ static fs::path GetDirectoryOfExecutable()
     if (path.is_absolute())
         return path.parent_path();
     else
-        return fs::absolute(path).parent_path();
+        return fs::canonical(fs::absolute(path)).parent_path();

[DoctorKraft]

i have build this change, but same no reload :/

[svranesevic]

Does -DSCRIPTS="dynamic" reloads properly ?
If so I assume problem lies somewhere within CMake part.

@Naios shine some light upon us :)

[DoctorKraft]

-DSCRIPTS="dynamic" doesn't work also ^^

[DoctorKraft]

For information i use

Clang 3.8
Cmake 3.8rc3 (tried also with cmake 3.0.2)
Boost 1.55

and this command
cmake ../../branch/master -DCMAKE_INSTALL_PREFIX=/home/***/binary/master -DWITH_WARNINGS=1 -DSCRIPTS="minimal-dynamic" -DWITH_DYNAMIC_LINKING=ON -DTOOLS=0

[Naios]

I currently don't have any idea about this, sorry.
I can look at this when I'm back home in 2 Weeks,
currently my capabilities are very limited.

You could try to enable trace logs for the hotswapping system so we know what's happening.

[ghost]

@DoctorKraft when you build it with cmake the -DCMAKE_INSTALL_PREFIX= path must be outside the bin directory in order to get the dynamic linking to work !!

[Naios]

Ah good catch @Skizooo, maybe we could add a CMake warning to the buildsystem,
in order to avoid such cases.

[ghost]

Yes you could, that's a great idea.

[DoctorKraft]

@Skizooo so if i resume, i need move all files in my -DCMAKE_INSTALL_PREFIX not bin folder ?
If yes, it's totally broken now, recompiler runs but doesn't reload and if i restart my worldserver no modification

[ghost]

@DoctorKraft if you move them now is useless, you must build with cmake again with the -DCMAKE_INSTALL_PREFIX path to another directory (you will call it for example "NewServer") , after you compiled everything you need to copy the worldserver / bnetserver from bin directory to the directory called "NewServer" and run them from there.

[DoctorKraft]

the -DCMAKE_INSTALL_PREFIX= path must be outside the bin directory

Sorry but i don't understand, i can't get -DCMAKE_INSTALL_PREFIX outside of bin folder because -DCMAKE_INSTALL_PREFIX create

• bin
• etc
• lib

can you give me a concrete example ?

[ghost]

@DoctorKraft i'm sorry for the misunderstanding, right now just copy the files that are inside the bin outside and start the server from there.

[DoctorKraft]

@Skizooo I have already tried with cmake run before. But nothing reload when i modify... I have look when i modify and reload, the recompiler create folder "scripts" in bin (It's not normal) because I have a good path in worldserver.conf. Normally script folder are in -DCMAKE_INSTALL_PREFIX with worldserver/bnetserver etc.. ? Why the recompiler create a folder scripts in bin after reload...

[Naios]

Make sure that:

• you specify an install location (CMAKE_INSTALL_PREFIX) outside your build directory
• and you need to make the install target so the binaries are installed into your chosen directory...

make install

... probably you missed the latter point.

[Naios]

@DoctorKraft Did you try out the configuration I described in the post above?
I'm very sure that this will fix your issues.

If so, let us know, so we can close this issue.

[DoctorKraft]

@Naios I know how to build, i resume because maybe i don't understand one point (i have a very bad english) x)

i have 3 directories

build
TrinityCore (source)
server

cd build
cmake ../TrinityCore -DCMAKE_INSTALL_PREFIX=/home/MYUSER/server/ -DWITH_WARNINGS=1 -DSCRIPTS="minimal-dynamic" -DWITH_DYNAMIC_LINKING=ON -DTOOLS=0
make ... && make install

now i have my binary in server/bin
i move content of my server/bin folder (worldserver etc..) in folder server

I'm right?

[Naios]

No, all content must stay inside the directory it was installed.
What's your reason for moving the content after installation?

[DoctorKraft]

Skizooo he said

i'm sorry for the misunderstanding, right now just copy the files that are inside the bin outside and start the server from there.

But when you say no moving, it's my first method and i don't have dynamic reload with this. The recompiler works perfect but i need reboot worldserver for reload lib with modification

[Naios]

Ah no. That's wrong.
And the reloader isn't working if you have to restart the whole worldserver.

Did you try to run the worldserver from various locations:

cd server

./bin/worldserver
# ... vs ..
cd bin
./worldserver

Maybe there are issues with relative pathing

[DoctorKraft]

I have tried both but it's the same no reload (i don't have error on start or when the recompiler runs). I need reboot worldserver for get modification in my spellscript.

Nobody has a linux for check if it's me or not?

[ghtyrant]

Can confirm this issue on Arch (running TC tag/6.2.4/21742). The library is being built correctly, the destructor of the old script is called, then constructor of the OLD(!) script runs. The old script is still being used afterwards for handling events (OnGossipHello, etc.).
Things I did to confirm this behaviour:

• I've started worldserver with a simple script containing TC_LOG_INFO in it's constructor:

my_test_class() : GameObjectScript("my_test")
{
    TC_LOG_INFO("scripts.custom", "Creating new my_test!");
}

• I've saved the script again to trigger a hotswap. The worldserver compiles my script and calls make install. I have confirmed this by looking at the modified time of libscripts_custom.so.
• "Creating new my_test!" appears in the worldservers log.
• Now I changed the TC_LOG_INFO line to TC_LOG_INFO("scripts.custom", "Creating new my_test - NEW SCRIPT!");
• The worldserver compiles my changed script and calls make install. I have confirmed that the new file is being installed to the correct directory by running strings libscripts_custom.so | grep NEW SCRIPT" - the .so contains my new string.
• The script gets reloaded, the OLD(!) string appears ("Creating new my_test!")

What works:

• The only mechanism affected by this bug is REloading. Triggering an UNload first, then letting the server load the script again works as expected (the actual new script is being used).
  • You can try this by renaming libscripts_custom.so in your scripts directory to libscripts_custom.so.tmp, waiting for the worldserver to unload it, then naming it back to libscripts_custom.so. worldserver will load the file, the correct string ("Creating new my_test - NEW SCRIPT!") appears in the logs.

So somehow, there's a difference between calling ProcessReloadScriptModule() and calling ProcessUnloadScriptModule(), waiting a few cycles and then calling ProcessLoadScriptModule().

ProcessReloadScriptModule() simply calls ProcessUnloadScriptModule(), then ProcessLoadScriptModule(), while keeping the script context when unloading. Forcing it to swap script context does not solve this issue.

Maybe someone with more insight to this problem can figure something out from my tests.

[ghtyrant]

I have implemented a workaround to this problem. The attached patch defers loading of libraries to the next cycle when calling ProcessReloadScriptModule().
No warranties that this will always work and not cause horrible, horrible issues. Do not use this in production. This is merely a workaround, not a solution to the problem.

diff --git a/src/server/game/Scripting/ScriptReloadMgr.cpp b/src/server/game/Scripting/ScriptReloadMgr.cpp
index b0c9b6821d..f26306656f 100644
--- a/src/server/game/Scripting/ScriptReloadMgr.cpp
+++ b/src/server/game/Scripting/ScriptReloadMgr.cpp
@@ -827,6 +827,10 @@ private:
     /// load/unload/reload requests of shared libraries.
     void DispatchModuleChanges()
     {
+        // Add libraries requesting a reload as workaround to #19373
+        _libraries_changed.insert(_libraries_reload_requested.begin(), _libraries_reload_requested.end());
+        _libraries_reload_requested.clear();
+
         // When there are no libraries to change return
         if (_libraries_changed.empty())
             return;
@@ -966,8 +970,8 @@ private:
 
     void ProcessReloadScriptModule(fs::path const& path)
     {
-        ProcessUnloadScriptModule(path, false);
-        ProcessLoadScriptModule(path);
+        ProcessUnloadScriptModule(path);
+        _libraries_reload_requested.insert(path);
     }
 
     void ProcessUnloadScriptModule(fs::path const& path, bool finish = true)
@@ -1409,6 +1413,7 @@ private:
 
     // Change requests to load or unload shared libraries
     std::unordered_set<fs::path /*path*/> _libraries_changed;
+    std::unordered_set<fs::path /*path*/> _libraries_reload_requested;
     // The timestamp which indicates the last time a library was changed
     uint32 _last_time_library_changed;

[Naios]

What happens when you enable caching for your platform?

#else // Posix
    #include <dlfcn.h>
-    // #define HOTSWAP_PLATFORM_REQUIRES_CACHING
+    #define HOTSWAP_PLATFORM_REQUIRES_CACHING
#endif

Can you post the logs when verbose logging for the hotswap system is enabled?
Because then the libraries have a different representation in the file system and we can
clearly say which library is loaded.

[DoctorKraft]

Well found @ghtyrant
@Naios, works with caching ! ;) You need the logs anyway?

[Naios]

No, I thought that it might lead to issues anyway overwriting the loading dll...
Since we are caching the shared libraries on all platforms now,
we could enable it by default and also get rid of the HOTSWAP_PLATFORM_REQUIRES_CACHING macro.

[DoctorKraft]

Ok nice ! You PR or i make it ?

[Naios]

Feel free to open a PR for it

[DoctorKraft]

Done ! Thanks @Naios for your help and others, the hotswap system is awesome for fastly work 👍

[Naios]

This should be resolved by #19465