Skip to content

Make ByteArrayPool thread-safe - #1091

Open
jimdroberts wants to merge 1 commit into
FirstGearGames:mainfrom
jimdroberts:fix/bytearraypool-thread-safety
Open

jimdroberts wants to merge 1 commit into
FirstGearGames:mainfrom
jimdroberts:fix/bytearraypool-thread-safety

Conversation

@jimdroberts

Copy link
Copy Markdown
Contributor

Problem

ByteArrayPool is a static, process-wide Queue<byte[]> with no synchronization. Transports that rent buffers on a socket thread and return them on the main thread corrupt it. The main thread also rents and stores through the same pool (Connection/Buffer.cs, PredictionManager, NetworkAnimator, LatencySimulator). The symptoms are:

  • the same array handed to two users at once, so one packet's bytes overwrite another's;
  • InvalidOperationException (Queue empty) from the Count > 0 / Dequeue race;
  • ArgumentException from a concurrent Enqueue during the queue's internal resize.

The bundled Synapse transport does this. IngressEngine.StartAsync runs ReceiveLoopAsync under Task.Run (Synapse/SynapseSocket/Transport/IngressEngine.cs L152). Delivered payloads raise SynapseManager.OnPayloadDelivered → PacketReceived (SynapseManager.cs L647-653) on that thread. Synapse/Core/ServerSocket.cs L355-361 and ClientSocket.cs L241-244 then construct a Packet, which calls ByteArrayPool.Retrieve (Synapse/Core/Supporting.cs L24). Those packets are disposed, via ByteArrayPool.Store (Supporting.cs L33), on the main thread in IterateIncoming (ServerSocket.cs L194-202, ClientSocket.cs L117-122).

Tugboat is not affected: LiteNetLib raises NetworkReceiveEvent from PollEvents on the main thread (Tugboat/Core/CommonSocket.cs L131-152), because Tugboat does not enable unsynced events. Third-party transports that receive on native or thread-pool callbacks and reuse ByteArrayPool, as ours does from msquic callbacks, are affected in the same way.

Cause

Runtime/Utility/Performance/ByteArrayPool.cs on main, L14 private static Queue<byte[]> _byteArrays:

  • Retrieve checks Count and then calls Dequeue (L23-24).
  • Store checks Count and then calls Enqueue (L44-46).

Neither is synchronized, and Queue<T> is not safe for concurrent use.

Fix

A private static lock guards both queue operations. Allocation and Array.Resize stay outside the lock. The public API and the pool's sizing behaviour are unchanged.

Evidence

This was reproduced outside Unity with a .NET 10 console app. It compiles the upstream ByteArrayPool.cs file as-is, and then the patched file. One task stands in for a socket thread: it calls Retrieve(1200) and hands the array to a ConcurrentQueue. Another task stands in for the main thread: it drains that queue with Store and also does its own Retrieve(512)/Store. The app tracks leased arrays by reference. Each run lasts 5 s:

upstream main    retrieves=  44170001 same-array-leased-twice=920707 exceptions=  59 ArgumentException,InvalidOperationException
patched (lock)   retrieves=  31326442 same-array-leased-twice=     0 exceptions=   0
upstream main    retrieves=  38018703 same-array-leased-twice=189070 exceptions=   8 ArgumentException,InvalidOperationException
patched (lock)   retrieves=  32540669 same-array-leased-twice=     0 exceptions=   0
upstream main    retrieves=  37380002 same-array-leased-twice=173357 exceptions=  40 ArgumentException,InvalidOperationException
patched (lock)   retrieves=  29939883 same-array-leased-twice=     0 exceptions=   0

The lower patched throughput comes from two threads doing nothing but hammering the pool. In real use the lock is uncontended, which is a Monitor enter/exit per call. The branch compiles cleanly with no new diagnostics (FishNet.Runtime, GameKit.Dependencies, Codegen).

Behavior change

None for single-threaded use.

ByteArrayPool is a static Queue<byte[]> with no synchronization. A transport
that retrieves buffers on a receive thread and stores them on the main thread,
such as the bundled Synapse transport, could hand the same array to two users
or throw from Queue. Guard Retrieve and Store with a lock; allocation stays
outside it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant