Use unbounded SemaphoreSlim

[Erarndt]

Fixes #

Context

This is a modification of #12113 that uses a SemaphoreSlim without a cap on the number of times it can be released. The previous code checked the count property to see if Release() needed to be called since calling it when there is a maximum specified can throw an exception if we're already at the max.

However, this pattern has a potential race condition if more than one thread tries to enqueue a packet at once. Multiple threads read 0 from CurrentCount and try to call Release() which causes an exception to be thrown. Additionally, the ARM64 architecture has a weaker memory model than x86/64 which can lead to unintended behavior. The .NET runtime maintains its own memory model rules that ensure correctness across platforms. Of note in this case is this section about order of memory operations.

"The effects of ordinary reads and writes can be reordered as long as that preserves single-thread consistency."

Since the runtime only needs to guarantee single-thread consistency, the read of something like CurrentCount could be read before the packet is enqueued, and this could lead to DrainPacketQueue never being signaled.

Changes Made

Testing

Notes

[Copilot]

Pull Request Overview

This PR replaces the problematic race condition-prone pattern of using a bounded SemaphoreSlim with count checking with an unbounded SemaphoreSlim in the NodeContext packet queue system. The change addresses thread safety issues that could occur when multiple threads attempt to enqueue packets simultaneously, particularly on ARM64 architectures with weaker memory models.

Key changes:

• Replace AutoResetEvent and CancellationTokenSource with unbounded SemaphoreSlim for packet queue signaling
• Convert packet draining from dedicated thread to async Task-based approach
• Add memory barriers around packet enqueue operations to ensure proper ordering
• Refactor shutdown logic to use proper resource disposal patterns

[Copilot]

Thread.MemoryBarrier() creates a full memory fence which can be expensive. Consider using Interlocked operations or volatile semantics if specific ordering guarantees are needed, as the .NET memory model already provides sufficient guarantees for most scenarios.

[Copilot]

Second Thread.MemoryBarrier() call may be unnecessary. The SemaphoreSlim.Release() method already provides appropriate memory ordering guarantees for signaling the waiting thread.

[Copilot]

Calling cts.Cancel() after the Task.WhenAny completes is unnecessary and potentially problematic. The CancellationTokenSource is already disposed in the using block, and cancellation should only be called if you need to interrupt the delay task early.

Suggested change

cts.Cancel();

[Copilot]

This condition duplicates exit logic that's already handled by IsExitPacket(). Consider consolidating the exit conditions or clearly documenting why NodeBuildComplete requires separate handling from other exit packets.

Suggested change

	if (packet is NodeBuildComplete)
	{
	return;
	}
	// If NodeBuildComplete packets with PrepareForReuse == true require special handling,
	// document the reason here. Otherwise, rely on IsExitPacket for all exit logic.

[SimaTian]

This is a non-trivial change and we already got burned. We will dig into it deeper.