fix(net): survive transient socket errors in the receive loop (2026-07-24 audit review)
WorldSession.NetReceiveLoop wrapped its entire while loop in a single try/catch, so ANY non-timeout SocketException permanently killed the background receive thread: the catch block at the loop's end was empty (misattributed the error to "socket closed during shutdown"), and the finally called _inboundQueue.Writer.TryComplete(), which silently and irrecoverably stopped all inbound processing for the rest of the session — no log line, no recovery path, and LiveSessionHost.Reconnect has zero production callers to notice. The realistic trigger is a well-known Windows UdpClient quirk: an ICMP "port unreachable" reply to an EARLIER Send (e.g. against a stale ACE session that already tore down its socket) surfaces as a WSAECONNRESET SocketException on this socket's NEXT, completely unrelated Receive call. NetClient.Receive already swallows the expected SocketError.TimedOut heartbeat case; anything else reaching WorldSession was a real, transient, per-datagram error being treated as session-fatal. Three changes, root-cause not a band-aid: - NetClient's constructor now disables SIO_UDP_CONNRESET reporting on Windows, so a delayed ICMP error can't poison receives at all. - NetReceiveLoop now catches SocketException PER ITERATION, logs it, and continues polling instead of exiting. The existing clean-shutdown paths (cancellation, ObjectDisposedException during Dispose) are unchanged — only the non-timeout-socket-error case that used to kill the loop is now recoverable. - NetClient.Receive no longer calls the ReceiveTimeout setter (a setsockopt syscall) on every single call — only when the requested timeout differs from the last-applied value, cached in a new field. This was an unrelated but adjacent finding (4x/sec syscall churn at the 250ms heartbeat cadence) in the same audit. No change to outbound wire behavior, ack cadence, heartbeat interval, or datagram ordering — this is purely receive-loop resilience. Tests: NetClientTests gained a SIO_UDP_CONNRESET construction smoke test and two ReceiveTimeout-caching tests. A new WorldSessionNetReceiveLoopResilienceTests drives the actual private NetReceiveLoop method (via the existing internal IWorldSessionTransport seam + reflection) with a scripted transport that throws a non-timeout SocketException on the first call, proving the loop survives it and keeps enqueueing subsequent datagrams — fully deterministic, no real sockets. Full solution suite green: 3204/3206 Core, 3462/3465 App, 552/552 Core.Net (skips pre-existing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit c72ce028a927e15b8a54a86bbd0661a723dc84ca)
This commit is contained in:
parent
c2cb83d11f
commit
cf25330458
4 changed files with 215 additions and 3 deletions
|
|
@ -0,0 +1,102 @@
|
|||
using System.Net;
|
||||
using System.Net.Sockets;
|
||||
using System.Reflection;
|
||||
|
||||
namespace AcDream.Core.Net.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// 2026-07-24 audit fix: <c>WorldSession.NetReceiveLoop</c> used to wrap its
|
||||
/// entire while loop in a single try/catch, so a non-timeout
|
||||
/// <see cref="SocketException"/> (classically Windows delivering a delayed
|
||||
/// WSAECONNRESET off an earlier Send's ICMP port-unreachable) exited the
|
||||
/// loop for good and silently killed all inbound processing for the rest of
|
||||
/// the session. These tests drive the actual private <c>NetReceiveLoop</c>
|
||||
/// method — via the internal <see cref="IWorldSessionTransport"/> seam and
|
||||
/// reflection, since the method has no public entry point — with a scripted
|
||||
/// transport that throws a non-timeout SocketException on the first call.
|
||||
/// No real sockets are involved, so the tests are deterministic.
|
||||
/// </summary>
|
||||
public sealed class WorldSessionNetReceiveLoopResilienceTests
|
||||
{
|
||||
[Fact]
|
||||
public void NetReceiveLoop_NonTimeoutSocketException_LogsAndKeepsReceiving()
|
||||
{
|
||||
var transport = new ScriptedTransport();
|
||||
var session = new WorldSession(
|
||||
new IPEndPoint(IPAddress.Loopback, 9000),
|
||||
transport);
|
||||
|
||||
MethodInfo loopMethod = typeof(WorldSession).GetMethod(
|
||||
"NetReceiveLoop", BindingFlags.NonPublic | BindingFlags.Instance)!;
|
||||
|
||||
// Deliberately does NOT redirect Console.Error: xUnit runs test
|
||||
// classes in this assembly in parallel by default, and Console.Error
|
||||
// is process-global mutable state, so swapping it out here could
|
||||
// race with another class's concurrently running test and either
|
||||
// swallow its output or restore the wrong writer. The behavioral
|
||||
// assertions below (call count + queue drain) fully cover the fix
|
||||
// without needing to observe the log line's exact text.
|
||||
var thread = new Thread(() => loopMethod.Invoke(session, null))
|
||||
{
|
||||
IsBackground = true,
|
||||
Name = "test.net-receive-loop",
|
||||
};
|
||||
thread.Start();
|
||||
|
||||
// The scripted transport throws SocketException on call 1, returns
|
||||
// one datagram on call 2, then throws ObjectDisposedException on
|
||||
// call 3 to end the loop the same way a real shutdown-time
|
||||
// disposal would. If the pre-fix bug were still present, the loop
|
||||
// would exit after call 1 and this thread would never terminate on
|
||||
// its own via the scripted ObjectDisposedException (call 3 would
|
||||
// never be reached).
|
||||
bool exited = thread.Join(TimeSpan.FromSeconds(5));
|
||||
Assert.True(exited, "NetReceiveLoop did not exit after the scripted ObjectDisposedException — " +
|
||||
"the non-timeout SocketException likely killed the loop before it reached call 3.");
|
||||
|
||||
// Proves the loop survived the first (non-timeout) SocketException
|
||||
// and went on to make a second AND third Receive() call, instead of
|
||||
// exiting after call 1 the way the pre-fix code did.
|
||||
Assert.Equal(3, transport.CallCount);
|
||||
|
||||
// The datagram handed back on call 2 must have made it into the
|
||||
// inbound queue — i.e. the loop's normal write path still runs
|
||||
// after recovering from the error.
|
||||
int processed = session.Tick();
|
||||
Assert.Equal(1, processed);
|
||||
}
|
||||
|
||||
private sealed class ScriptedTransport : IWorldSessionTransport
|
||||
{
|
||||
private int _calls;
|
||||
|
||||
public int CallCount => _calls;
|
||||
|
||||
public void Send(ReadOnlySpan<byte> datagram) { }
|
||||
|
||||
public void Send(IPEndPoint remote, ReadOnlySpan<byte> datagram) { }
|
||||
|
||||
public byte[]? Receive(TimeSpan timeout, out IPEndPoint? from)
|
||||
{
|
||||
int n = Interlocked.Increment(ref _calls);
|
||||
from = null;
|
||||
switch (n)
|
||||
{
|
||||
case 1:
|
||||
// Simulates a non-timeout SocketException, e.g. Windows'
|
||||
// WSAECONNRESET off a stale peer's ICMP port-unreachable.
|
||||
throw new SocketException((int)SocketError.ConnectionReset);
|
||||
case 2:
|
||||
from = new IPEndPoint(IPAddress.Loopback, 9000);
|
||||
return new byte[] { 0xAA, 0xBB, 0xCC, 0xDD };
|
||||
default:
|
||||
// Simulates NetClient having been disposed out from
|
||||
// under the loop during shutdown — the pre-existing,
|
||||
// still-preserved silent-exit path.
|
||||
throw new ObjectDisposedException(nameof(ScriptedTransport));
|
||||
}
|
||||
}
|
||||
|
||||
public void Dispose() { }
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue