Skip to content

Fix server hanging after a packet error from a disconnected client - #285

Open
magaflaca wants to merge 1 commit into
Pryaxis:general-develfrom
magaflaca:fix/main-loop-crash-on-message-error
Open

magaflaca wants to merge 1 commit into
Pryaxis:general-develfrom
magaflaca:fix/main-loop-crash-on-message-error

Conversation

@magaflaca

Copy link
Copy Markdown

Problem

On TShock 6.2.1 / Terraria 1.4.5.8 a server can hang right after a player disconnects: the console stops (TShock then reports "Unable to write to log as log has been disposed"), the port stays open and accepts connections, but nobody is served. The process never exits, so restart loops and service managers don't restart it.

Two issues combine:

  1. NetMessage.LogMessageError throws. Terraria 1.4.5.8 added this method and CheckBytes calls it when GetData throws. It builds the message with Netplay.Clients[bufferIndex].Socket.GetRemoteAddress() without a null check. If the slot's client already disconnected (RemoteClient.Reset() set Socket = null) while one of its packets was still being processed, a harmless packet error becomes a NullReferenceException that escapes CheckBytes and ends the main game loop. Terraria 1.4.4.9 and 1.4.5.6 only wrote the error to the console here, without touching the socket.
  2. The process stays alive. Program.RunGame catches the exception and returns, and TSAPI unloads the plugins, but Netplay.Disconnect is never set. Netplay.ServerLoop keeps running and the foreground BroadcastThread keeps the process alive indefinitely.

Production stack trace:

System.NullReferenceException: Object reference not set to an instance of an object.
   at Terraria.NetMessage.mfwh_LogMessageError(Int32 bufferIndex, String id, Exception exception)
   at Terraria.NetMessage.LogMessageError(Int32 bufferIndex, String id, Exception exception)
   at Terraria.NetMessage.mfwh_CheckBytes(Int32 bufferIndex)
   at Terraria.Netplay.UpdateInMainThread()
   at Terraria.Main.mfwh_DoUpdate(GameTime& gameTime)
   ...

Issue: Pryaxis/TShock#3324

Fix

  • Hook NetMessage.LogMessageError and log the same message through Invariant.Assert, using Socket?.GetRemoteAddress().
  • Set Netplay.Disconnect = true once WindowsLaunch.Main returns, so the network threads stop and the process exits after an abnormal end of the game loop. A normal shutdown already sets it, so nothing changes there.

Changelog

  • Fixed the server hanging forever (port open, nobody served, process not exiting) when a packet from a client that just disconnected failed to process. Terraria 1.4.5.8's new LogMessageError threw on the missing socket and ended the game loop.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant