Skip to content

Fix data race on WebSocketClient::_ctx (concurrent disconnect vs send/isConnected)#6

Open
voicetel wants to merge 1 commit into
amigniter:mainfrom
voicetel:fix/ctx-thread-safety
Open

Fix data race on WebSocketClient::_ctx (concurrent disconnect vs send/isConnected)#6
voicetel wants to merge 1 commit into
amigniter:mainfrom
voicetel:fix/ctx-thread-safety

Conversation

@voicetel

Copy link
Copy Markdown

Problem

WebSocketClient::_ctx (the std::shared_ptr<WebSocketContext>) is read and
written from different threads with no synchronization:

  • read by isConnected(), sendBinary(), sendMessage(), and the
    setXxxCallback() setters — these are typically called from an application's
    media/sender thread;
  • written by connect() (_ctx = ctx) and disconnect() (_ctx.reset()),
    called from the teardown/control thread.

This is a data race on the shared_ptr itself. It is easy to hit when a sender
thread is active while another thread tears the client down (e.g. a connection
that disconnects while audio is still being sent), and ThreadSanitizer reports it:

data race  std::__shared_ptr<WebSocketContext>::operator bool()
  Read  by sender thread:  WebSocketClient::isConnected()   (if (_ctx))
  Write by control thread: WebSocketClient::disconnect()    (_ctx.reset())

Fix

Make every _ctx access atomic using the C++11 std::atomic_load /
std::atomic_store / std::atomic_exchange free functions for shared_ptr.
disconnect() atomically swaps _ctx out and then calls stop() (which joins
the event thread) on the local copy, outside the atomic op — so a concurrent
sender either observes the old context (kept alive by its own shared_ptr copy)
or null, never a torn pointer, and the join can't deadlock a callback thread.

Notes:

  • C++11-only (no <atomic>-on-shared_ptr member needed), so the class's existing
    move semantics and public API are unchanged.
  • No behavioral change for single-threaded use.

Verification

Built the transport standalone under ThreadSanitizer and drove it against a mock
server through a lifecycle matrix (connect / send / far-end drop / rapid restart /
reconnect-churn / disconnect-while-sending). Before: the _ctx race above is
reported; after: clean.

isConnected()/sendBinary()/sendMessage() (callable from the media thread) read
_ctx while connect()/disconnect() write it from another thread, with no
synchronization -- a data race (ThreadSanitizer-confirmed) on the shared_ptr.
Use C++11 atomic shared_ptr ops for every _ctx access; disconnect() atomically
swaps _ctx out and calls stop() (the event-thread join) on the local copy,
outside the swap, so it cannot deadlock a callback 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.

2 participants