From e5fc1ab7ed7e31e471cdc427ca4404f82f21abb1 Mon Sep 17 00:00:00 2001 From: Suyash Date: Fri, 18 Sep 2026 02:44:03 +0000 Subject: [PATCH] fix(win): stop Disconnect from freezing the UI, and a 100% CPU spin on link loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related problems around teardown on Windows: 1. `BluetoothWrapper::disconnect()` took `_connectorMtx` before closing the socket, but a worker thread holds that same lock while blocked in `recv()`. Closing the socket is what releases that thread, so the UI thread waited on the lock — Disconnect froze the window until the 2.5s recv timeout expired, and closing the app while a read was in flight hung it. On Windows, close first, then take the lock; other platforms keep the old order. 2. `_socket` was written by the UI thread (disconnect) while a worker thread read it inside `send()`/`recv()` — a data race that could hand `recv()` a closed or recycled handle. It is now `std::atomic`, swapped through a single `_closeSocket()`. 3. `recv()` returning 0 (graceful close by the headphones) was treated as a successful read of an empty buffer, so `BluetoothWrapper::_readMessage()` looped forever looking for a start marker — one core pegged until the app was killed. Both the connector and the wrapper now treat it as a disconnect. Also loops `send()` until the whole frame is out: a partial send left the device waiting for the rest of a command that never arrived. Co-Authored-By: Claude Opus 5 --- Client/BluetoothWrapper.cpp | 19 ++++++- Client/windows/WindowsBluetoothConnector.cpp | 60 ++++++++++++++------ Client/windows/WindowsBluetoothConnector.h | 9 ++- 3 files changed, 68 insertions(+), 20 deletions(-) diff --git a/Client/BluetoothWrapper.cpp b/Client/BluetoothWrapper.cpp index cd3cb74..23f6dfc 100644 --- a/Client/BluetoothWrapper.cpp +++ b/Client/BluetoothWrapper.cpp @@ -48,10 +48,21 @@ void BluetoothWrapper::connect(const std::string& addr) void BluetoothWrapper::disconnect() noexcept { +#if defined(_WIN32) + // Close the link BEFORE taking the lock. A worker thread can hold _connectorMtx while blocked in + // recv(), and closing the socket is exactly what wakes it up - taking the lock first deadlocks the + // UI thread until the recv timeout expires, and hangs the app on exit. The Windows connector's + // disconnect() is safe to call concurrently with an in-flight recv(). + this->_connector->disconnect(); + std::lock_guard guard(this->_connectorMtx); + this->_seqNumber = 0; + this->_leftoverBytes.clear(); +#else std::lock_guard guard(this->_connectorMtx); this->_seqNumber = 0; this->_leftoverBytes.clear(); this->_connector->disconnect(); +#endif } @@ -117,12 +128,18 @@ CommandSerializer::Message BluetoothWrapper::_readMessage() else { numRecvd = this->_connector->recv(buf, sizeof(buf)); + if (numRecvd <= 0) + { + // A connector that reports 0 bytes (peer closed) would otherwise spin this loop at + // 100% CPU forever, since no marker can ever be found in an empty buffer. + throw RecoverableException("The headphones closed the connection.", true); + } } size_t messageStart = 0; size_t messageEnd = numRecvd; - for (size_t i = 0; i < numRecvd; i++) + for (size_t i = 0; i < static_cast(numRecvd); i++) { if (buf[i] == START_MARKER) { diff --git a/Client/windows/WindowsBluetoothConnector.cpp b/Client/windows/WindowsBluetoothConnector.cpp index 6967f36..cfb1c10 100644 --- a/Client/windows/WindowsBluetoothConnector.cpp +++ b/Client/windows/WindowsBluetoothConnector.cpp @@ -1,5 +1,7 @@ #include "WindowsBluetoothConnector.h" +#include + void WSAStartupWrapper() { int iResult; @@ -50,8 +52,7 @@ void WindowsBluetoothConnector::connect(const std::string& addrStr) } // A failed connect() can leave the socket unusable - recreate it before trying the v2 UUID. - ::closesocket(this->_socket); - this->_socket = INVALID_SOCKET; + this->_closeSocket(); this->_initSocket(); if (_tryConnect(SONY_UUID_V2, sab)) @@ -66,25 +67,57 @@ void WindowsBluetoothConnector::connect(const std::string& addrStr) WindowsBluetoothConnector::~WindowsBluetoothConnector() { - if (this->_socket != INVALID_SOCKET) + this->_closeSocket(); +} + +void WindowsBluetoothConnector::_closeSocket() noexcept +{ + SOCKET sock = this->_socket.exchange(INVALID_SOCKET); + if (sock != INVALID_SOCKET) { - ::closesocket(this->_socket); + ::shutdown(sock, SD_BOTH); + ::closesocket(sock); } } int WindowsBluetoothConnector::send(char* buf, size_t length) { - auto bytesSent = ::send(this->_socket, buf, length, 0); - if (bytesSent == SOCKET_ERROR) + SOCKET sock = this->_socket.load(); + if (sock == INVALID_SOCKET) { - throw RecoverableException("Couldn't send (" + std::to_string(WSAGetLastError()) + ")", true); + throw RecoverableException("Not connected", true); + } + + // send() is free to accept only part of the buffer; keep going until the whole frame is out, + // otherwise the device sees a truncated command and never answers. + size_t total = 0; + while (total < length) + { + int chunk = static_cast((std::min)(length - total, static_cast(0x7fffffff))); + int sent = ::send(sock, buf + total, chunk, 0); + if (sent == SOCKET_ERROR) + { + throw RecoverableException("Couldn't send (" + std::to_string(WSAGetLastError()) + ")", true); + } + total += static_cast(sent); } - return bytesSent; + return static_cast(total); } int WindowsBluetoothConnector::recv(char* buf, size_t length) { - auto bytesReceived = ::recv(this->_socket, buf, length, 0); + SOCKET sock = this->_socket.load(); + if (sock == INVALID_SOCKET) + { + throw RecoverableException("Not connected", true); + } + + auto bytesReceived = ::recv(sock, buf, static_cast(length), 0); + if (bytesReceived == 0) + { + // Graceful close by the headphones. Without this the caller loops on an empty buffer forever. + throw RecoverableException("The headphones closed the connection.", true); + } if (bytesReceived == SOCKET_ERROR) { int err = WSAGetLastError(); @@ -142,13 +175,8 @@ std::vector WindowsBluetoothConnector::getConnectedDevices() void WindowsBluetoothConnector::disconnect() noexcept { - if (this->_socket != INVALID_SOCKET) - { - this->_connected = false; - shutdown(this->_socket, SD_BOTH); - closesocket(this->_socket); - this->_socket = INVALID_SOCKET; - } + this->_connected = false; + this->_closeSocket(); } bool WindowsBluetoothConnector::isConnected() noexcept diff --git a/Client/windows/WindowsBluetoothConnector.h b/Client/windows/WindowsBluetoothConnector.h index 824b8e1..690fd12 100644 --- a/Client/windows/WindowsBluetoothConnector.h +++ b/Client/windows/WindowsBluetoothConnector.h @@ -34,8 +34,11 @@ class WindowsBluetoothConnector final : public IBluetoothConnector std::string _wstringToUtf8(const std::wstring& wstr); bool _tryConnect(const char* uuid, SOCKADDR_BTH& sab); - SOCKET _socket = INVALID_SOCKET; - std::atomic _connected = false; - SonyProtocolVersion _protocolVersion = SonyProtocolVersion::V1; + // disconnect() runs on the UI thread while a worker thread can be inside recv(), so the handle + // itself has to be atomic; closing it is what makes that blocked recv() return. + std::atomic _socket{ INVALID_SOCKET }; + std::atomic _connected{ false }; + std::atomic _protocolVersion{ SonyProtocolVersion::V1 }; void _initSocket(); + void _closeSocket() noexcept; }; \ No newline at end of file