NorthstarLauncher icon indicating copy to clipboard operation
NorthstarLauncher copied to clipboard

Shut down server on crash and add server performance warning

Open rolelessweapon opened this issue 1 year ago • 6 comments

Shuts down server on crash, disconnecting all clients. image

rolelessweapon avatar Sep 11 '22 00:09 rolelessweapon

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? ^^

GeckoEidechse avatar Sep 11 '22 00:09 GeckoEidechse

Yes.

rolelessweapon avatar Sep 11 '22 00:09 rolelessweapon

Reference from source: https://pastebin.com/NAZGMGps

rolelessweapon avatar Sep 11 '22 00:09 rolelessweapon

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

p0358 avatar Sep 11 '22 08:09 p0358

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

p0358 avatar Sep 11 '22 08:09 p0358

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)?

pg9182 avatar Sep 11 '22 23:09 pg9182