From ba8bab2282b0fa24c0ccea24e6dd77f1e556a791 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 15 Aug 2017 19:26:38 +0200 Subject: [PATCH 1/6] Factor out socket flag selection into GetBlockingFlagIfNeeded() No real changes, just refactor wxProtocol ctor to use a new function that can be reused elsewhere too. --- include/wx/socket.h | 10 ++++++++++ src/common/protocol.cpp | 4 +--- src/common/socket.cpp | 9 +++++++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/include/wx/socket.h b/include/wx/socket.h index daded4bf05..4a66716984 100644 --- a/include/wx/socket.h +++ b/include/wx/socket.h @@ -225,6 +225,16 @@ public: bool IsNoWait() const { return ((m_flags & wxSOCKET_NOWAIT) != 0); } wxSocketType GetType() const { return m_type; } + // Helper returning wxSOCKET_NONE if non-blocking sockets can be used, i.e. + // the socket is being created in the main thread and the event loop is + // running, or wxSOCKET_BLOCK otherwise. + // + // This is an internal function used only by wxWidgets itself, user code + // should decide if it wants blocking sockets or not and use the + // appropriate style instead of using it (but wxWidgets has to do it like + // this for compatibility with the original network classes behaviour). + static int GetBlockingFlagIfNeeded(); + private: friend class wxSocketClient; friend class wxSocketServer; diff --git a/src/common/protocol.cpp b/src/common/protocol.cpp index f6250252c3..ffe0e0f086 100644 --- a/src/common/protocol.cpp +++ b/src/common/protocol.cpp @@ -65,9 +65,7 @@ wxIMPLEMENT_ABSTRACT_CLASS(wxProtocol, wxObject); wxProtocol::wxProtocol() #if wxUSE_SOCKETS // Only use non blocking sockets if we can dispatch events. - : wxSocketClient((wxIsMainThread() && wxApp::IsMainLoopRunning() - ? wxSOCKET_NONE - : wxSOCKET_BLOCK) | wxSOCKET_WAITALL) + : wxSocketClient(wxSocketClient::GetBlockingFlagIfNeeded() | wxSOCKET_WAITALL) #endif { m_lastError = wxPROTO_NOERR; diff --git a/src/common/socket.cpp b/src/common/socket.cpp index 24214b1859..37cd53783a 100644 --- a/src/common/socket.cpp +++ b/src/common/socket.cpp @@ -25,6 +25,7 @@ #include "wx/socket.h" #ifndef WX_PRECOMP + #include "wx/app.h" #include "wx/object.h" #include "wx/string.h" #include "wx/intl.h" @@ -928,6 +929,14 @@ wxSocketError wxSocketBase::LastError() const return m_impl->GetError(); } +/* static */ +int wxSocketBase::GetBlockingFlagIfNeeded() +{ + return wxIsMainThread() && wxApp::IsMainLoopRunning() + ? wxSOCKET_NONE + : wxSOCKET_BLOCK; +} + // -------------------------------------------------------------------------- // Basic IO calls // -------------------------------------------------------------------------- From 40774e1ccd2bad20a3f184941825f9416d7742a6 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 15 Aug 2017 13:48:11 +0200 Subject: [PATCH 2/6] Use blocking sockets from non-main threads for passive FTP too This extends the changes of d421373c2eee777fd1300fcfb56971f9248ba382 to the case of passive FTP, which created wxSocketClient directly and so didn't use the correct flags when used from a worker thread. See #17937. --- src/common/ftp.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/common/ftp.cpp b/src/common/ftp.cpp index 4644099901..84ad5f28f9 100644 --- a/src/common/ftp.cpp +++ b/src/common/ftp.cpp @@ -647,7 +647,11 @@ wxSocketBase *wxFTP::GetPassivePort() addr.Hostname(hostaddr); addr.Service(port); - wxSocketClient *client = new wxSocketClient(); + // If we're used from a worker thread or can't dispatch events even though + // we're in the main one, we can't use non-blocking sockets. + wxSocketClient* const + client = new wxSocketClient(wxSocketClient::GetBlockingFlagIfNeeded()); + if ( !client->Connect(addr) ) { m_lastError = wxPROTO_CONNERR; From 8d66bfd7ef4f128ffa94d226cd5e8774f89916dc Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 15 Aug 2017 19:30:42 +0200 Subject: [PATCH 3/6] Use blocking server socket in non-main threads for active FTP This is similar to the previous commit, but for active FTP connections. wxSocketServer was also created directly in this case, without using wxProtocol ctor, so wxSOCKET_BLOCK must be explicitly specified when creating it from worker thread, just as it was already done in d421373c2eee777fd1300fcfb56971f9248ba382 for the other connections and in the previous commit for passive FTP ones. See #17937. --- src/common/ftp.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/common/ftp.cpp b/src/common/ftp.cpp index 84ad5f28f9..d1cd21b60b 100644 --- a/src/common/ftp.cpp +++ b/src/common/ftp.cpp @@ -582,7 +582,13 @@ wxSocketBase *wxFTP::GetActivePort() addrNew.AnyAddress(); addrNew.Service(0); // pick an open port number. - wxSocketServer *sockSrv = new wxSocketServer(addrNew); + wxSocketServer* const + sockSrv = new wxSocketServer + ( + addrNew, + wxSocketServer::GetBlockingFlagIfNeeded() + ); + if (!sockSrv->IsOk()) { // We use IsOk() here to see if everything is ok From 8a29f958a186af1a20237dfbb92cfd15fb8bd317 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 15 Aug 2017 13:53:15 +0200 Subject: [PATCH 4/6] Detect any attempt to use non-blocking socket from worker threads This doesn't work, as non-blocking sockets implementation requires dispatching events generated by them, which is only possible from the main thread event loop, and it's better to be upfront about it rather than failing mysteriously later. --- interface/wx/socket.h | 6 ++++++ src/common/socket.cpp | 10 +++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/interface/wx/socket.h b/interface/wx/socket.h index 0de0c81e43..36ad0f87d2 100644 --- a/interface/wx/socket.h +++ b/interface/wx/socket.h @@ -289,6 +289,12 @@ public: /** Constructor. + Notice that if the object is created from a worker thread or if it is + created from the main thread but the event loop is not running, @a + flags parameter @em must include ::wxSOCKET_BLOCK as non-blocking + sockets require dispatching events, which can only be done in the main + thread. + @param flags Socket flags (See wxSocketBase::SetFlags()) */ diff --git a/src/common/socket.cpp b/src/common/socket.cpp index 37cd53783a..5fb8c291ae 100644 --- a/src/common/socket.cpp +++ b/src/common/socket.cpp @@ -25,7 +25,6 @@ #include "wx/socket.h" #ifndef WX_PRECOMP - #include "wx/app.h" #include "wx/object.h" #include "wx/string.h" #include "wx/intl.h" @@ -1975,6 +1974,15 @@ bool wxSocketBase::SetLocal(const wxIPV4address& local) wxSocketClient::wxSocketClient(wxSocketFlags flags) : wxSocketBase(flags, wxSOCKET_CLIENT) { + // Notice that we don't check for a running event loop here, unlike in + // GetBlockingFlagIfNeeded() because it is common to create the sockets + // before the event loop is entered and we shouldn't break existing code + // doing this as it can still work correctly if it only uses non-blocking + // sockets once the event loop is running. + wxASSERT_MSG( (flags & wxSOCKET_BLOCK) || wxIsMainThread(), + wxS("Non-blocking sockets may only be created ") + wxS("in the main thread") ); + m_initialRecvBufferSize = m_initialSendBufferSize = -1; } From 2e3e265a8b23e4ec10e15c4dc56099315421d88b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 15 Aug 2017 14:15:33 +0200 Subject: [PATCH 5/6] Remove unnecessary checks for INVALID_SOCKET in MSW code wxSocketManager::Install_Callback() and Uninstall_Callback() are only called for successfully created sockets, so there should be no need for these checks and there are none in the Unix version. --- src/msw/sockmsw.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/msw/sockmsw.cpp b/src/msw/sockmsw.cpp index 2330a33a6c..791f18a49b 100644 --- a/src/msw/sockmsw.cpp +++ b/src/msw/sockmsw.cpp @@ -267,15 +267,12 @@ void wxSocketMSWManager::Install_Callback(wxSocketImpl *socket_, { wxSocketImplMSW * const socket = static_cast(socket_); - if (socket->m_fd != INVALID_SOCKET) - { /* We could probably just subscribe to all events regardless * of the socket type, but MS recommends to do it this way. */ long lEvent = socket->m_server? FD_ACCEPT : (FD_READ | FD_WRITE | FD_CONNECT | FD_CLOSE); gs_WSAAsyncSelect(socket->m_fd, hWin, socket->m_msgnumber, lEvent); - } } /* @@ -286,10 +283,7 @@ void wxSocketMSWManager::Uninstall_Callback(wxSocketImpl *socket_, { wxSocketImplMSW * const socket = static_cast(socket_); - if (socket->m_fd != INVALID_SOCKET) - { gs_WSAAsyncSelect(socket->m_fd, hWin, socket->m_msgnumber, 0); - } } // set the wxBase variable to point to our wxSocketManager implementation From dadb7b9fb0ae63d0ef7cc66c6433e22c95ee240b Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 15 Aug 2017 14:22:43 +0200 Subject: [PATCH 6/6] Don't register async notifications for MSW blocking sockets Under MSW calling UnblockAndRegisterWithEventLoop() for blocking sockets is not only useless, but actually harmful when the socket is used from a worker thread (which is the common case for blocking sockets), as it means that the main thread will be receiving notifications for the socket events and modifying the socket object while it's being used from the other thread, resulting in data races and general brokenness. This is similar to e18c8fd29a10b1b87ea5cff7c5b3a16fe5b32690 for Unix sockets. Closes #17937. --- include/wx/msw/private/sockmsw.h | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/include/wx/msw/private/sockmsw.h b/include/wx/msw/private/sockmsw.h index 5c4e565d20..bbcad77bca 100644 --- a/include/wx/msw/private/sockmsw.h +++ b/include/wx/msw/private/sockmsw.h @@ -51,9 +51,25 @@ private: virtual void UnblockAndRegisterWithEventLoop() wxOVERRIDE { - // no need to make the socket non-blocking, Install_Callback() will do - // it - wxSocketManager::Get()->Install_Callback(this); + if ( GetSocketFlags() & wxSOCKET_BLOCK ) + { + // Counter-intuitively, we make the socket non-blocking even in + // this case as it is necessary e.g. for Read() to return + // immediately if there is no data available. However we must not + // install a callback for it as blocking sockets don't use any + // events and generating them would actually be harmful (and not + // just useless) as they would be dispatched by the main thread + // while this blocking socket can be used from a worker one, so it + // would result in data races and other unpleasantness. + unsigned long trueArg = 1; + ioctlsocket(m_fd, FIONBIO, &trueArg); + } + else + { + // No need to make the socket non-blocking, Install_Callback() will + // do it as a side effect of calling WSAAsyncSelect(). + wxSocketManager::Get()->Install_Callback(this); + } } int m_msgnumber;