NorthstarLauncher
NorthstarLauncher copied to clipboard
Shut down server on crash and add server performance warning
Shuts down server on crash, disconnecting all clients.
Just so I understand correctly, the purpose behind this PR is that the client gets the server shut-down signal so instead of freezing and timing out, they get disconnected properly from server, right? ^^
Yes.
Reference from source: https://pastebin.com/NAZGMGps
Overall it's a good idea, but I'd suggest employing a few changes similar to what I did myself below in TFOR:
if (Util::IsServerAlive())
{
spdlog::error("Game server was running, shutting it down to kick out the connected clients...");
MemoryAddress(enginedllBaseAddress).Offset(0x6398A8).PatchString("#DISCONNECT_SERVERCRASHED", sizeof("#DISCONNECT_SERVERSHUTTINGDOWN"));
auto sv = static_cast<uint64_t>(enginedllBaseAddress + 0x27C5C80);
auto CBaseServer_Shutdown = reinterpret_cast<void(__fastcall*)(uint64_t thisptr)>(enginedllBaseAddress + 0x20EDF0); // it checks sv.IsActive() inside anyways
CBaseServer_Shutdown(sv);
}
- consider showing some log line before (and maybe after), as this is an operation that can potentially cause another crash itself (but that requires
sv.IsActive()
check which the func that's called checks itself before kicking clients; and it doesn't matter that much in Northstar to log it as a crash dump is already saved at that point) - consider patching
#DISCONNECT_SERVERSHUTTINGDOWN
string to something like#DISCONNECT_SERVERCRASHED
so that people know the server encountered an issue and don't assume the server owner is playing with them - consider naming the function and maybe argument (assigning it to variable first), so it's more clear/readable what's being called
Not saying they're a must, but they're my personal recommendations
But overall I tested this method and it works, so I approve 👍 definitely better than being stuck on resuming connection for several seconds if we can tell the clients it's not gonna happen, the risk of subsequent crash should hopefully be reasonably low here
Looks like a good change overall, and I second @p0358's recommendations.
Does this handle the engine error crashes (it doesn't seem like it would, since those manifest as a messagebox which blocks the game loop)?
@rolelessweapon is this something that is going to be worked on?