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