Drop UDP packets with no client instead of killing the server - #347
Conversation
|
The crash this fixes: an 800-bot join died in ServerUdpQueue.DedicatedThreadLoop with a null client, and the process shut down with Exception during Process. After the null check, the same 1000-bot join stayed up. The probe finished at 977 players and 126,104 chunks. That run was also testing an 8192 dirty-block cap, which was too expensive and is not part of this PR. No UDP-thread null reference in that log. |
|
/rebase |
An 800-bot join queued a packet whose client was already gone. The send and the packet recycle both dereferenced it, and that exception on the UDP thread stopped the process.
|
Rebased
Your local branch is now behind the remote. Run |
5c1f455 to
463d615
Compare
Pixnop
left a comment
There was a problem hiding this comment.
Approved. Thanks for chasing this one down: it is a real crash, and it is ours, not vanilla's.
Vanilla survived a null client. The NullReferenceException was thrown inside its try/catch and logged once per packet. Our pooling change (8956ec7, shipped since v1.22.5-stratum.1) added result.client.IsSinglePlayerClient in the finally and in the TTL branch, outside the catch, on a raw thread, so the same input now takes the process down. Refusing the packet in QueuePacket is the right place to stop it, and dropping is always safe: a packet with no client had nowhere to go.
Checked here: hunk counters match, the patch applies to a pristine baseline, both build passes are green, 49/49 scenarios.
Three small things, none blocking:
- The second guard, after
TryDequeue, cannot run. The queue is private, its onlyEnqueuesits behind your new guard, andConcurrentQueuenever yields null. That hunk also removes and re-adds the vanilla lineif (result.creationTime > num). I would drop it and leave the vanilla line alone. - The drop is silent, where vanilla logged an error per packet. A log line would be the wrong fix, since a client kicked mid-burst would print one per stale packet. A counter in the perf report keeps the signal that some caller is still sending to a player who left.
- The description quotes "Exception during Process". That line comes from
ServerThread.Process, which the UdpSending thread does not go through. If you still have the log from the 800-bot join, its real last lines would help whoever hits this next.
I ran /rebase, so the branch sits on current indev. Pull with --rebase before your next push.
Zaldaryon
left a comment
There was a problem hiding this comment.
Approved. The null-client crash comes from Stratum's post-dequeue/finally access on the raw UDP sender path. Rejecting the packet in QueuePacket prevents that unguarded access, and dropping it is safe because there is no recipient. Pixnop's exact-head review reports that the patch applies, both Release builds pass, and the 49 scenarios pass. GitHub reports no status checks for this branch.
Non-blocking: the post-dequeue null guard is unreachable through the sole guarded enqueue path. Keeping it adds dead code and changes a vanilla hunk without covering another caller.
Summary
SendPacketBlockingand again in the packet-recyclefinally, and that second exception stopped the process.What happened
On this PC, an 800-bot join with the send queue on died here:
The log then shut the server down with "Exception during Process". The same join without this null client reached the profiler.
Test plan
/probe profiler 60