Skip to content

Conversation

@dead4d5a
Copy link

Title: Fix send queue deadlocks and improve stability during disconnects (no API breaking changes)

Description:

Summary

This PR improves reliability of the .NET/Native (non-WebGL) implementation by:

  • Replacing the existing send-queue + lock approach with a single async send pump that never blocks the thread with synchronous waits.
  • Ensuring pending send operations complete (success or fault) even if the socket is closed/aborted while sends are queued.
  • Hardening main-thread coroutine dispatch to avoid depending on a specific SynchronizationContext being available.

The goal is to prevent hangs/freezes and “await never completes” scenarios seen during poor network conditions, disconnects, or teardown.

Problems addressed

1) Potential deadlocks / hangs in send path

The current implementation uses a mixed approach (locks + Monitor.TryEnter(m_Socket) + Task.Wait(...) inside an async method). In practice, a synchronous wait inside an async send pipeline can stall the calling thread and/or hang when the socket enters a closing/aborted state while sends are queued.

This PR moves to a single async send queue:

  • All sends are enqueued into one ConcurrentQueue.
  • A single pump task drains the queue and awaits SendAsync(...) normally.
  • Each queued send has a completion Task that resolves when that send completes or fails.

This avoids synchronous waits and minimizes lock contention.

2) Queued sends can wait forever after close

If the connection drops while sends are queued, callers awaiting Send(...) / SendText(...) can get stuck if nothing completes those tasks.

This PR ensures:

  • When the socket closes (Receive loop ends / cleanup runs), all pending sends are failed with a consistent exception.
  • If the socket is not in Open, new sends fail quickly rather than silently queueing.

3) Main-thread dispatch robustness

MainThreadUtil.Run(...) previously relied on posting to a captured SynchronizationContext. In some Unity runners / initialization orders, the context can be null or not associated with the Unity main loop.

This PR changes Run(...) to enqueue coroutines and start them during Update(), ensuring they always start from the Unity main thread. Exceptions while starting coroutines are caught and logged (matching a “low drama” error-handling expectation).

Compatibility / behavior

  • No public type removals.
  • Existing method signatures remain.
  • Connect() behavior remains the same as upstream (still blocks until the receive loop ends). This PR does not change Connect() semantics for existing call sites.
  • WebGL path unchanged.

Notes on design

  • The send pump is guarded so only one instance drains the queue at a time.
  • Send tasks are completed with RunContinuationsAsynchronously to avoid running user continuations inline on the pump.
  • Cleanup is idempotent to avoid double-dispose races during teardown.

Testing

Tested in Unity by:

  • Connecting to a WS endpoint, sending bursts of text/binary concurrently.
  • Toggling network connectivity during active sends.
  • Repeated connect → send → disconnect cycles and scene reloads while dispatching DispatchMessageQueue() on the main thread.

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