fix(win): stop Disconnect from freezing the UI, and a 100% CPU spin on link loss

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<size_t>(numRecvd); i++)
 		{
 			if (buf[i] == START_MARKER)
 			{

Client/windows/WindowsBluetoothConnector.cpp

@@ -1,5 +1,7 @@
 #include "WindowsBluetoothConnector.h"
 
+#include <algorithm>
+
 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<int>((std::min)(length - total, static_cast<size_t>(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<size_t>(sent);
 	}
-	return bytesSent;
+	return static_cast<int>(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<int>(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<BluetoothDevice> 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

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<bool> _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> _socket{ INVALID_SOCKET };
+	std::atomic<bool> _connected{ false };
+	std::atomic<SonyProtocolVersion> _protocolVersion{ SonyProtocolVersion::V1 };
 	void _initSocket();
+	void _closeSocket() noexcept;
 };