Merge branch 'blocking-sockets-fixes'

Closes #17937.

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
+        if ( GetSocketFlags() & wxSOCKET_BLOCK )
-        // it
+        {
-        wxSocketManager::Get()->Install_Callback(this);
+            // 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;

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;

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())
     */

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
@@ -647,7 +653,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;

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()
+    : wxSocketClient(wxSocketClient::GetBlockingFlagIfNeeded() | wxSOCKET_WAITALL)
-                        ? wxSOCKET_NONE
-                        : wxSOCKET_BLOCK) | wxSOCKET_WAITALL)
 #endif
 {
     m_lastError = wxPROTO_NOERR;

src/common/socket.cpp

@@ -928,6 +928,14 @@ wxSocketError wxSocketBase::LastError() const
     return m_impl->GetError();
 }
 
+/* static */
+int wxSocketBase::GetBlockingFlagIfNeeded()
+{
+    return wxIsMainThread() && wxApp::IsMainLoopRunning()
+            ? wxSOCKET_NONE
+            : wxSOCKET_BLOCK;
+}
+
 // --------------------------------------------------------------------------
 // Basic IO calls
 // --------------------------------------------------------------------------
@@ -1966,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;
 }

src/msw/sockmsw.cpp

@@ -267,15 +267,12 @@ void wxSocketMSWManager::Install_Callback(wxSocketImpl *socket_,
 {
     wxSocketImplMSW * const socket = static_cast<wxSocketImplMSW *>(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<wxSocketImplMSW *>(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