From c437abc3c56a80d6fe73935f0406483dfc97e92c Mon Sep 17 00:00:00 2001 From: PA Dev Bot <344001113+pa-devbot@users.noreply.github.com> Date: Sat, 31 Oct 2026 07:17:00 +0000 Subject: [PATCH 1/5] Fixes to ClientSocket_WinSocket The Windows socket implementation was stale and had several bugs: - connect() called ThreadPool::blocking_dispatch(), which no longer exists (the API is dispatch_now_blocking()); the file did not compile. - Connection resets were not detected: when the peer closed the connection, recv() == 0 looped forever at 1 ms, and on socket errors (e.g. WSAECONNRESET) the receive thread exited, but the state stayed CONNECTED and no callback fired. Fire the on_disconnect() callback added in 'Sockets: add Listener::on_disconnect() hook' when the connection is lost, and return the socket to NOT_RUNNING. - close() only set a flag: the socket was never closed and the state was stuck at DESTRUCTING forever, so the object could not be reused. close() now waits for the receive thread to exit (including any in-flight listener callback), closes the socket, and returns the socket to NOT_RUNNING so connect() can be called again. It is safe to call multiple times, and must not be called from within a listener callback. - send() counted and advanced by the requested bytes instead of the bytes actually sent, so a partial send silently dropped the remainder and reported full success. - The connect-failure paths called m_lock.unlock() while a std::unique_lock on the same mutex was still active (double unlock). - WSAEINVAL in the connect loop was retried forever; it is now treated as a fatal error like the other connect errors. A socket whose previous connection has ended cannot be reconnected, so connect() creates a fresh socket each time. Not wired in yet: ClientSocket.h still selects ClientSocket_Qt on Windows. --- Common/Cpp/Sockets/ClientSocket_WinSocket.h | 182 ++++++++++++++------ 1 file changed, 131 insertions(+), 51 deletions(-) diff --git a/Common/Cpp/Sockets/ClientSocket_WinSocket.h b/Common/Cpp/Sockets/ClientSocket_WinSocket.h index ad24dbcb9e..fdbac5cb45 100644 --- a/Common/Cpp/Sockets/ClientSocket_WinSocket.h +++ b/Common/Cpp/Sockets/ClientSocket_WinSocket.h @@ -7,6 +7,7 @@ #ifndef PokemonAutomation_ClientSocket_WinSocket_H #define PokemonAutomation_ClientSocket_WinSocket_H +#include #include #include #include "Common/Cpp/Concurrency/Mutex.h" @@ -25,65 +26,114 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ public: ClientSocket_WinSocket(ThreadPool& thread_pool) : m_thread_pool(thread_pool) - , m_socket(::socket(AF_INET, SOCK_STREAM, 0)) - { - u_long non_blocking = 1; - if (ioctlsocket(m_socket, FIONBIO, &non_blocking)){ -// cout << "ioctlsocket() Failed" << endl; - close_socket(); - return; - } - } + {} virtual ~ClientSocket_WinSocket(){ close(); - m_thread.wait_and_ignore_exceptions(); - close_socket(); } + + // Stops the receive thread, closes the socket, and returns the socket to + // State::NOT_RUNNING so "connect()" can be called again. This is safe to + // call multiple times. + // + // This blocks until the receive thread has exited, so it must not be + // called from within a listener callback (which runs on that thread). virtual void close() noexcept override{ + AsyncTask thread; { - std::lock_guard lg1(m_lock); - m_state.store(State::DESTRUCTING, std::memory_order_relaxed); + std::lock_guard lg(m_lock); + if (m_state.load(std::memory_order_relaxed) == State::NOT_RUNNING && !m_thread){ + // Never connected, or already fully closed. + return; + } + m_state.store(State::DESTRUCTING, std::memory_order_release); + m_cv.notify_all(); + thread = std::move(m_thread); + } + + thread.wait_and_ignore_exceptions(); + + { + std::lock_guard lg(m_lock); + close_socket_locked(); + m_state.store(State::NOT_RUNNING, std::memory_order_release); m_cv.notify_all(); } } virtual void connect(const std::string& address, uint16_t port) override{ - std::lock_guard lg1(m_lock); - if (m_state.load(std::memory_order_relaxed) != State::NOT_RUNNING){ + std::string error; + { + std::lock_guard lg(m_lock); + if (m_state.load(std::memory_order_relaxed) != State::NOT_RUNNING){ + return; + } + + // A socket whose previous connection has ended cannot be + // reconnected, so always start from a fresh socket. + close_socket_locked(); + + SOCKET socket = ::socket(AF_INET, SOCK_STREAM, 0); + if (socket == INVALID_SOCKET){ + error = "socket() failed: WSA Error Code: " + std::to_string(WSAGetLastError()); + }else{ + u_long non_blocking = 1; + if (ioctlsocket(socket, FIONBIO, &non_blocking) != 0){ + error = "ioctlsocket() failed: WSA Error Code: " + std::to_string(WSAGetLastError()); + ::closesocket(socket); + }else{ + m_socket.store(socket, std::memory_order_release); + } + } + if (!error.empty()){ + m_error = error; + return; + } + + m_state.store(State::CONNECTING, std::memory_order_relaxed); + } + if (!error.empty()){ + m_listeners.run_method(&Listener::on_connect_finished, error); return; } + try{ - m_state.store(State::CONNECTING, std::memory_order_relaxed); - m_thread = m_thread_pool.blocking_dispatch([=, this]{ + m_thread = m_thread_pool.dispatch_now_blocking([=, this]{ thread_loop(address, port); }); }catch (...){ - m_state.store(State::NOT_RUNNING, std::memory_order_relaxed); + { + std::lock_guard lg(m_lock); + m_error = "Failed to start the receive thread."; + close_socket_locked(); + m_state.store(State::NOT_RUNNING, std::memory_order_relaxed); + } + m_listeners.run_method(&Listener::on_connect_finished, m_error); throw; } } virtual size_t send(const void* data, size_t bytes) override{ - if (m_socket == INVALID_SOCKET){ - return 0; - } - - constexpr int BLOCK_SIZE = (int)1 << 30; const char* ptr = (const char*)data; size_t sent = 0; while (bytes > 0 && state() == State::CONNECTED){ - size_t current = std::min(bytes, BLOCK_SIZE); - int current_sent = ::send(m_socket, ptr, (int)current, 0); + SOCKET socket = m_socket.load(std::memory_order_acquire); + if (socket == INVALID_SOCKET){ + return sent; + } + + constexpr size_t BLOCK_SIZE = (size_t)1 << 30; + int current = (int)std::min(bytes, BLOCK_SIZE); + int current_sent = ::send(socket, ptr, current, 0); if (current_sent != SOCKET_ERROR){ - sent += current; - if ((size_t)current_sent < current){ + sent += current_sent; + if (current_sent != current){ return sent; } - ptr += current; - bytes -= current; + ptr += current_sent; + bytes -= current_sent; continue; } @@ -110,17 +160,39 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ private: - void close_socket(){ - if (m_socket == INVALID_SOCKET){ + // Closes the underlying socket, if any. Requires "m_lock" to be held. + void close_socket_locked(){ + SOCKET socket = m_socket.exchange(INVALID_SOCKET, std::memory_order_acquire); + if (socket == INVALID_SOCKET){ return; } - int ret = closesocket(m_socket); - if (ret == 0){ - return; + if (::closesocket(socket) != 0){ + try{ + std::cout << "Failed closesocket(): WSA Error Code = " + std::to_string(WSAGetLastError()) << std::endl; + }catch (...){} + } + } + + // A connection ended on its own (the connect attempt failed, the server + // closed the connection, or a socket error occurred). This returns the + // socket to State::NOT_RUNNING so "connect()" can be called again, and + // fires the appropriate callback outside of "m_lock". + // Requires "lg" to be locked. It unlocks it. + void connection_lost_locked( + std::unique_lock& lg, + const std::string& error_message, + bool was_connected + ){ + m_error = error_message; + m_state.store(State::NOT_RUNNING, std::memory_order_release); + close_socket_locked(); + lg.unlock(); + + if (was_connected){ + m_listeners.run_method(&Listener::on_disconnect, error_message); + }else{ + m_listeners.run_method(&Listener::on_connect_finished, error_message); } - try{ - std::cout << "Failed closesocket(): WSA Error Code = " + std::to_string(ret) << std::endl; - }catch (...){} } void thread_loop(const std::string& address, uint16_t port){ @@ -151,7 +223,12 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ return; } - if (::connect(m_socket, (struct sockaddr*)&server, sizeof(server)) != SOCKET_ERROR){ + SOCKET socket = m_socket.load(std::memory_order_acquire); + if (socket == INVALID_SOCKET){ + return; + } + + if (::connect(socket, (struct sockaddr*)&server, sizeof(server)) != SOCKET_ERROR){ break; } @@ -162,19 +239,12 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ case WSAEISCONN: goto Connected; case WSAEWOULDBLOCK: - case WSAEINVAL: break; case WSAETIMEDOUT: - m_state.store(State::NOT_RUNNING, std::memory_order_relaxed); - m_error = "Connection timed out."; - m_lock.unlock(); - m_listeners.run_method(&Listener::on_connect_finished, m_error); + connection_lost_locked(lg, "Connection timed out.", false); return; default: - m_state.store(State::NOT_RUNNING, std::memory_order_relaxed); - m_error = "WSA Error Code: " + std::to_string(error); - m_lock.unlock(); - m_listeners.run_method(&Listener::on_connect_finished, m_error); + connection_lost_locked(lg, "WSA Error Code: " + std::to_string(error), false); return; } @@ -200,7 +270,13 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ if (state == State::DESTRUCTING){ return; } - bytes = ::recv(m_socket, buffer, BUFFER_SIZE, 0); + + SOCKET socket = m_socket.load(std::memory_order_acquire); + if (socket == INVALID_SOCKET){ + return; + } + + bytes = ::recv(socket, buffer, BUFFER_SIZE, 0); if (bytes == SOCKET_ERROR){ error = WSAGetLastError(); } @@ -216,13 +292,17 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ return; } + if (bytes == 0){ + connection_lost_locked(lg, "Connection closed by server.", true); + return; + } + if (bytes == SOCKET_ERROR){ -// cout << "error = " << error << endl; switch (error){ case WSAEWOULDBLOCK: break; default: - m_error = "WSA Error Code: " + std::to_string(error); + connection_lost_locked(lg, "Connection lost: WSA Error Code: " + std::to_string(error), true); return; } } @@ -235,7 +315,7 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ private: ThreadPool& m_thread_pool; - const SOCKET m_socket; + std::atomic m_socket{INVALID_SOCKET}; std::string m_error; From f88f312a494425e27959b72dbda7fadc67e3dbe4 Mon Sep 17 00:00:00 2001 From: PA Dev Bot <344001113+pa-devbot@users.noreply.github.com> Date: Sat, 31 Oct 2026 09:39:02 +0000 Subject: [PATCH 2/5] Sockets: call WSAStartup in ClientSocket_WinSocket Nothing in the program ever calls WSAStartup, so the WinSocket backend was relying on Qt having initialized Winsock first. If this backend is used before any Qt networking call, the first socket() fails with WSA_NOT_INITIALIZED (10093). Call WSAStartup exactly once, before the first socket is created. WSACleanup is deliberately never called: it is a process-wide, reference-counted operation and Qt may still be using Winsock. --- Common/Cpp/Sockets/ClientSocket_WinSocket.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Common/Cpp/Sockets/ClientSocket_WinSocket.h b/Common/Cpp/Sockets/ClientSocket_WinSocket.h index fdbac5cb45..d86439cab7 100644 --- a/Common/Cpp/Sockets/ClientSocket_WinSocket.h +++ b/Common/Cpp/Sockets/ClientSocket_WinSocket.h @@ -8,6 +8,7 @@ #define PokemonAutomation_ClientSocket_WinSocket_H #include +#include #include #include #include "Common/Cpp/Concurrency/Mutex.h" @@ -73,6 +74,8 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ // reconnected, so always start from a fresh socket. close_socket_locked(); + ensure_wsa_started(); + SOCKET socket = ::socket(AF_INET, SOCK_STREAM, 0); if (socket == INVALID_SOCKET){ error = "socket() failed: WSA Error Code: " + std::to_string(WSAGetLastError()); @@ -160,6 +163,24 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ private: + // Winsock has to be started before any of its functions can be used, and + // nothing else in the program calls WSAStartup, so call it exactly once + // before the first socket is created. We deliberately never call + // WSACleanup: it is a process-wide, reference-counted operation, and + // other parts of the process (Qt) may be using Winsock as well. + static void ensure_wsa_started(){ + static std::once_flag flag; + std::call_once(flag, [](){ + WSADATA wsa_data; + int ret = WSAStartup(MAKEWORD(2, 2), &wsa_data); + if (ret != 0){ + try{ + std::cout << "WSAStartup() failed: WSA Error Code = " << ret << std::endl; + }catch (...){} + } + }); + } + // Closes the underlying socket, if any. Requires "m_lock" to be held. void close_socket_locked(){ SOCKET socket = m_socket.exchange(INVALID_SOCKET, std::memory_order_acquire); From 77f978d5641fabe61c7d9aba446ca52f52c3cb3a Mon Sep 17 00:00:00 2001 From: PA Dev Bot <344001113+pa-devbot@users.noreply.github.com> Date: Sat, 31 Oct 2026 11:22:23 +0000 Subject: [PATCH 3/5] Sockets: fix WSAEINVAL on connect retries in ClientSocket_WinSocket Calling connect() again while a non-blocking connect is in progress is not safe on Windows: once the attempt fails, the second connect() returns WSAEINVAL (10022) instead of starting a new attempt. This made rapid connect/disconnect cycles fail intermittently with 'WSA Error Code: 10022' - the real failure (usually a reset or refusal while the peer is still tearing down the previous connection) was hidden. Do the portable thing instead of retrying connect(): call connect() once, wait for the socket to become writable with select(), and read the result with getsockopt(SO_ERROR). The real error is now reported (refused -> 'Connection refused.', timeout -> 'Connection timed out.', anything else -> its WSA code). --- Common/Cpp/Sockets/ClientSocket_WinSocket.h | 79 +++++++++++++++------ 1 file changed, 58 insertions(+), 21 deletions(-) diff --git a/Common/Cpp/Sockets/ClientSocket_WinSocket.h b/Common/Cpp/Sockets/ClientSocket_WinSocket.h index d86439cab7..14bf83b61d 100644 --- a/Common/Cpp/Sockets/ClientSocket_WinSocket.h +++ b/Common/Cpp/Sockets/ClientSocket_WinSocket.h @@ -238,42 +238,79 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ server.sin_addr.s_addr = inet_addr(address.c_str()); // Connect + SOCKET socket = m_socket.load(std::memory_order_acquire); + if (socket == INVALID_SOCKET){ + return; + } + + bool in_progress = false; while (true){ State state = m_state.load(std::memory_order_relaxed); if (state == State::DESTRUCTING){ return; } - SOCKET socket = m_socket.load(std::memory_order_acquire); - if (socket == INVALID_SOCKET){ - return; + if (!in_progress){ + int ret = ::connect(socket, (struct sockaddr*)&server, sizeof(server)); + if (ret != SOCKET_ERROR){ + break; + } + + int error = WSAGetLastError(); +// cout << "error = " << error << endl; + if (error != WSAEWOULDBLOCK && error != WSAEINPROGRESS){ + if (error == WSAETIMEDOUT){ + connection_lost_locked(lg, "Connection timed out.", false); + }else{ + connection_lost_locked(lg, "WSA Error Code: " + std::to_string(error), false); + } + return; + } + in_progress = true; } - if (::connect(socket, (struct sockaddr*)&server, sizeof(server)) != SOCKET_ERROR){ - break; + // Once the connect is in progress, calling connect() again + // is not safe on Windows: when the attempt fails, the + // second connect() returns WSAEINVAL instead of starting a + // new attempt. So wait for the socket to become writable + // and read the result with getsockopt(SO_ERROR). + fd_set write_set; + FD_ZERO(&write_set); + FD_SET(socket, &write_set); + timeval tv; + tv.tv_sec = 0; + tv.tv_usec = 10000; + int ret = ::select(0, nullptr, &write_set, nullptr, &tv); + if (ret == SOCKET_ERROR){ + int error = WSAGetLastError(); + if (error != WSAEINTR){ + connection_lost_locked(lg, "select() failed: WSA Error Code: " + std::to_string(error), false); + return; + } + continue; + } + if (ret == 0){ + continue; } - int error = WSAGetLastError(); -// cout << "error = " << error << endl; - - switch (error){ - case WSAEISCONN: - goto Connected; - case WSAEWOULDBLOCK: + int so_error = 0; + int so_len = sizeof(so_error); + if (getsockopt(socket, SOL_SOCKET, SO_ERROR, (char*)&so_error, &so_len) == SOCKET_ERROR){ + so_error = WSAGetLastError(); + } + if (so_error == 0){ break; - case WSAETIMEDOUT: + } + if (so_error == WSAETIMEDOUT){ connection_lost_locked(lg, "Connection timed out.", false); - return; - default: - connection_lost_locked(lg, "WSA Error Code: " + std::to_string(error), false); - return; + }else if (so_error == WSAECONNREFUSED){ + connection_lost_locked(lg, "Connection refused.", false); + }else{ + connection_lost_locked(lg, "WSA Error Code: " + std::to_string(so_error), false); } - - m_cv.wait_for(lg, std::chrono::milliseconds(10)); - + return; } -Connected: m_state.store(State::CONNECTED, std::memory_order_relaxed); } From bcdd880b7e13defc502b30df6f1e82e66c9b0015 Mon Sep 17 00:00:00 2001 From: PA Dev Bot <344001113+pa-devbot@users.noreply.github.com> Date: Sat, 31 Oct 2026 12:07:41 +0000 Subject: [PATCH 4/5] Sockets: start the receive thread with dispatch() Now that dispatch() spawns threads as needed, the receive thread no longer needs dispatch_now_blocking(). The blocking variant can also deadlock if connect() is called from a thread that is itself running on a capped pool, since it blocks until a pool thread is free. --- Common/Cpp/Sockets/ClientSocket_WinSocket.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Common/Cpp/Sockets/ClientSocket_WinSocket.h b/Common/Cpp/Sockets/ClientSocket_WinSocket.h index 14bf83b61d..373b3d3a2e 100644 --- a/Common/Cpp/Sockets/ClientSocket_WinSocket.h +++ b/Common/Cpp/Sockets/ClientSocket_WinSocket.h @@ -100,8 +100,13 @@ class ClientSocket_WinSocket final : public AbstractClientSocket{ return; } + // Use the plain dispatch() rather than dispatch_now_blocking(): + // connect() may be called from a thread that is itself running on + // this pool, and blocking here until a pool thread is free could + // deadlock a capped pool. The returned AsyncTask still lets + // close() join the receive thread. try{ - m_thread = m_thread_pool.dispatch_now_blocking([=, this]{ + m_thread = m_thread_pool.dispatch([=, this]{ thread_loop(address, port); }); }catch (...){ From 19f116f1ec18d47050cdfe2191e34dbd2e3cc30a Mon Sep 17 00:00:00 2001 From: Alexander Yee Date: Sat, 31 Oct 2026 05:35:02 -0700 Subject: [PATCH 5/5] Switch default TCP implementation to Windows. --- Common/Cpp/Sockets/ClientSocket.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Common/Cpp/Sockets/ClientSocket.h b/Common/Cpp/Sockets/ClientSocket.h index 011ebc6a00..a4748630ed 100644 --- a/Common/Cpp/Sockets/ClientSocket.h +++ b/Common/Cpp/Sockets/ClientSocket.h @@ -10,11 +10,11 @@ #ifdef _WIN32 -//#include "ClientSocket_WinSocket.h" -#include "ClientSocket_Qt.h" +#include "ClientSocket_WinSocket.h" +//#include "ClientSocket_Qt.h" namespace PokemonAutomation{ -// using ClientSocket = ClientSocket_WinSocket; - using ClientSocket = ClientSocket_Qt; + using ClientSocket = ClientSocket_WinSocket; +// using ClientSocket = ClientSocket_Qt; } #else #include "ClientSocket_POSIX.h"