Repository navigation
Make ByteArrayPool thread-safe - #1091
Open
jimdroberts wants to merge 1 commit into
Open
jimdroberts wants to merge 1 commit into
jimdroberts wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ByteArrayPoolis a static, process-wideQueue<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:InvalidOperationException(Queue empty) from theCount > 0/Dequeuerace;ArgumentExceptionfrom a concurrentEnqueueduring the queue's internal resize.The bundled Synapse transport does this.
IngressEngine.StartAsyncrunsReceiveLoopAsyncunderTask.Run(Synapse/SynapseSocket/Transport/IngressEngine.csL152). Delivered payloads raiseSynapseManager.OnPayloadDelivered→PacketReceived(SynapseManager.csL647-653) on that thread.Synapse/Core/ServerSocket.csL355-361 andClientSocket.csL241-244 then construct aPacket, which callsByteArrayPool.Retrieve(Synapse/Core/Supporting.csL24). Those packets are disposed, viaByteArrayPool.Store(Supporting.csL33), on the main thread inIterateIncoming(ServerSocket.csL194-202,ClientSocket.csL117-122).Tugboat is not affected: LiteNetLib raises
NetworkReceiveEventfromPollEventson the main thread (Tugboat/Core/CommonSocket.csL131-152), because Tugboat does not enable unsynced events. Third-party transports that receive on native or thread-pool callbacks and reuseByteArrayPool, as ours does from msquic callbacks, are affected in the same way.Cause
Runtime/Utility/Performance/ByteArrayPool.cson main, L14private static Queue<byte[]> _byteArrays:RetrievechecksCountand then callsDequeue(L23-24).StorechecksCountand then callsEnqueue(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.Resizestay 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.csfile as-is, and then the patched file. One task stands in for a socket thread: it callsRetrieve(1200)and hands the array to aConcurrentQueue. Another task stands in for the main thread: it drains that queue withStoreand also does its ownRetrieve(512)/Store. The app tracks leased arrays by reference. Each run lasts 5 s: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.