Cleanup of wxSocket::_Wait():

- Rename to DoWait() to avoid symbols starting with underscores.
 - Added comments explaining how does it work.
 - Remove the pointless timeout manipulations: GSocket::Select() doesn't
   use timeout (any more?) anyhow.

Also pass GSOCK_LOST_FLAG to DoWait() from WaitForWrite() for the same reasons
it is done in WaitForRead().


git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@56275 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775

include/wx/socket.h

@@ -137,12 +137,26 @@ public:
   wxSocketBase& Write(const void *buffer, wxUint32 nbytes);
   wxSocketBase& WriteMsg(const void *buffer, wxUint32 nbytes);
 
-  void InterruptWait() { m_interrupt = true; }
+  // all Wait() functions wait until their condition is satisfied or the
+  // timeout expires; if seconds == -1 (default) then m_timeout value is used
+  //
+  // it is also possible to call InterruptWait() to cancel any current Wait()
+
+  // wait for anything at all to happen with this socket
   bool Wait(long seconds = -1, long milliseconds = 0);
+
+  // wait until we can read from or write to the socket without blocking
+  // (notice that this does not mean that the operation will succeed but only
+  // that it will return immediately)
   bool WaitForRead(long seconds = -1, long milliseconds = 0);
   bool WaitForWrite(long seconds = -1, long milliseconds = 0);
+
+  // wait until the connection is terminated
   bool WaitForLost(long seconds = -1, long milliseconds = 0);
 
+  void InterruptWait() { m_interrupt = true; }
+
+
   wxSocketFlags GetFlags() const { return m_flags; }
   void SetFlags(wxSocketFlags flags);
   void SetTimeout(long seconds);
@@ -182,7 +196,17 @@ private:
   // low level IO
   wxUint32 _Read(void* buffer, wxUint32 nbytes);
   wxUint32 _Write(const void *buffer, wxUint32 nbytes);
-  bool     _Wait(long seconds, long milliseconds, wxSocketEventFlags flags);
+
+  // wait until the given flags are set for this socket or the given timeout
+  // (or m_timeout) expires
+  //
+  // notice that GSOCK_LOST_FLAG is always taken into account but the return
+  // value depends on whether it is included in flags or not: if it is, and the
+  // connection is indeed lost, true is returned, but if it isn't then the
+  // function returns false in this case
+  //
+  // false is always returned if we returned because of the timeout expiration
+  bool DoWait(long seconds, long milliseconds, wxSocketEventFlags flags);
 
   // pushback buffer
   void     Pushback(const void *buffer, wxUint32 size);
@@ -203,7 +227,7 @@ private:
   bool          m_closed;           // was the other end closed?
                                     // (notice that m_error is also set then)
   wxUint32      m_lcount;           // last IO transaction size
-  unsigned long m_timeout;          // IO timeout value
+  unsigned long m_timeout;          // IO timeout value in seconds
   wxList        m_states;           // stack of states
   bool          m_interrupt;        // interrupt ongoing wait operations?
   bool          m_beingDeleted;     // marked for delayed deletion?

src/common/socket.cpp

@@ -714,75 +714,65 @@ wxSocketBase& wxSocketBase::Discard()
 // timeout elapses. The polling loop runs the event loop so that
 // this won't block the GUI.
 
-bool wxSocketBase::_Wait(long seconds,
+bool
-                         long milliseconds,
+wxSocketBase::DoWait(long seconds, long milliseconds, wxSocketEventFlags flags)
-                         wxSocketEventFlags flags)
 {
-    GSocketEventFlags result;
+    wxCHECK_MSG( m_socket, false, "can't wait on invalid socket" );
-    long timeout; // in ms
 
-    // Set this to true to interrupt ongoing waits
+    // This can be set to true from Interrupt() to exit this function a.s.a.p.
     m_interrupt = false;
 
-    // Check for valid socket
-    if (!m_socket)
-        return false;
 
-    // Check for valid timeout value.
+    // Use either the provided timeout or the default timeout value associated
-    if (seconds != -1)
+    // with this socket.
-        timeout = seconds * 1000 + milliseconds;
+    //
-    else
+    // TODO: allow waiting forever, see #9443
-        timeout = m_timeout * 1000;
+    const long timeout = seconds == -1 ? m_timeout * 1000
+                                       : seconds * 1000 + milliseconds;
+    const wxMilliClock_t timeEnd = wxGetLocalTimeMillis() + timeout;
 
-    // Get the active event loop
+    // Get the active event loop which we'll use for the message dispatching
-    wxEventLoopBase * const eventLoop = wxEventLoop::GetActive();
+    // when running in the main thread
+    wxEventLoopBase *eventLoop;
+    if ( wxIsMainThread() )
+    {
+        eventLoop = wxEventLoop::GetActive();
 
 #ifdef __WXMSW__
-    wxASSERT_MSG( !wxIsMainThread() || eventLoop,
+        wxASSERT_MSG( eventLoop,
                       "Sockets won't work without a running event loop" );
 #endif // __WXMSW__
-
+    }
-    // Wait in an active polling loop.
+    else // in worker thread
-    //
-    // NOTE: We duplicate some of the code in OnRequest, but this doesn't
-    //   hurt. It has to be here because the (GSocket) event might arrive
-    //   a bit delayed, and it has to be in OnRequest as well because we
-    //   don't know whether the Wait functions are being used.
-    //
-    // Do this at least once (important if timeout == 0, when
-    // we are just polling). Also, if just polling, do not yield.
-
-    const wxMilliClock_t time_limit = wxGetLocalTimeMillis() + timeout;
-    bool done = false;
-    bool valid_result = false;
-
-    if (!eventLoop)
     {
-        // This is used to avoid a busy loop on wxBase - having a select
+        // We never dispatch messages from threads other than the main one.
-        // timeout of 50 ms per iteration should be enough.
+        eventLoop = NULL;
-        if (timeout > 50)
-            m_socket->SetTimeout(50);
-        else
-            m_socket->SetTimeout(timeout);
     }
 
-    while (!done)
+    // Wait in an active polling loop: notice that the loop is executed at
+    // least once, even if timeout is 0 (i.e. polling).
+    bool gotEvent = false;
+    for ( ;; )
     {
+        // We always stop waiting when the connection is lost as it doesn't
+        // make sense to continue further, even if GSOCK_LOST_FLAG is not
+        // specified in flags to wait for.
+        const GSocketEventFlags
             result = m_socket->Select(flags | GSOCK_LOST_FLAG);
 
-        // Incoming connection (server) or connection established (client)
+        // Incoming connection (server) or connection established (client)?
         if ( result & GSOCK_CONNECTION_FLAG )
         {
             m_connected = true;
             m_establishing = false;
-            valid_result = true;
+            gotEvent = true;
             break;
         }
 
-        // Data available or output buffer ready
+        // Data available or output buffer ready?
         if ( (result & GSOCK_INPUT_FLAG) || (result & GSOCK_OUTPUT_FLAG) )
         {
-            valid_result = true;
+            gotEvent = true;
             break;
         }
 
@@ -791,83 +781,73 @@ bool wxSocketBase::_Wait(long seconds,
         {
             m_connected = false;
             m_establishing = false;
-            valid_result = ((flags & GSOCK_LOST_FLAG) != 0);
+            if ( flags & GSOCK_LOST_FLAG )
+                gotEvent = true;
             break;
         }
 
+        if ( m_interrupt )
+            break;
+
         // Wait more?
-        long time_left = wxMilliClockToLong(time_limit - wxGetLocalTimeMillis());
+        const wxMilliClock_t timeNow = wxGetLocalTimeMillis();
-        if ((!timeout) || (time_left <= 0) || (m_interrupt))
+        if ( timeNow >= timeEnd )
-            done = true;
+            break;
-        else
+
-        {
         if ( eventLoop )
         {
-                // from the main thread itself we have to run the event loop to
+            // Dispatch the events when we run in the main thread and have an
-                // let the events (including the GUI events and the low-level
+            // active event loop: without this sockets don't work at all under
-                // (not wxWidgets) events from GSocket) be processed but from
+            // MSW as socket flags are only updated when socket messages are
-                // another thread it is enough to just call wxThread::Yield()
+            // processed.
-                // which will give away the rest of our time slice: the
-                // explanation is that the events will be processed by the main
-                // thread anyhow, but we don't want to eat the CPU time
-                // uselessly while sitting in the loop waiting for the data
-                if ( wxIsMainThread() )
-                {
             if ( eventLoop->Pending() )
                 eventLoop->Dispatch();
         }
 #if wxUSE_THREADS
-                else
+        else // no event loop or waiting in another thread
+        {
+            // We're busy waiting but at least give up the rest of our current
+            // time slice.
             wxThread::Yield();
+        }
 #endif // wxUSE_THREADS
     }
-            else
-            {
-                // If there's less than 50 ms left, just call select with that timeout.
-                if (time_left < 50)
-                    m_socket->SetTimeout(time_left);
-            }
-        }
-    }
 
-    // Set timeout back to original value (we overwrote it for polling)
+    return gotEvent;
-    if (!eventLoop)
-        m_socket->SetTimeout(m_timeout*1000);
-
-    return valid_result;
 }
 
 bool wxSocketBase::Wait(long seconds, long milliseconds)
 {
-    return _Wait(seconds, milliseconds, GSOCK_INPUT_FLAG |
+    return DoWait(seconds, milliseconds,
+            GSOCK_INPUT_FLAG |
             GSOCK_OUTPUT_FLAG |
             GSOCK_CONNECTION_FLAG |
-                                        GSOCK_LOST_FLAG);
+            GSOCK_LOST_FLAG
+        );
 }
 
 bool wxSocketBase::WaitForRead(long seconds, long milliseconds)
 {
-    // Check pushback buffer before entering _Wait
+    // Check pushback buffer before entering DoWait
     if ( m_unread )
         return true;
 
-    // Note that GSOCK_INPUT_LOST has to be explicitly passed to
+    // Note that GSOCK_INPUT_LOST has to be explicitly passed to DoWait
-    // _Wait because of the semantics of WaitForRead: a return
+    // because of the semantics of WaitForRead: a return value of true means
-    // value of true means that a GSocket_Read call will return
+    // that a GSocket_Read call will return immediately, not that there is
-    // immediately, not that there is actually data to read.
+    // actually data to read.
-
+    return DoWait(seconds, milliseconds, GSOCK_INPUT_FLAG | GSOCK_LOST_FLAG);
-    return _Wait(seconds, milliseconds, GSOCK_INPUT_FLAG | GSOCK_LOST_FLAG);
 }
 
 
 bool wxSocketBase::WaitForWrite(long seconds, long milliseconds)
 {
-    return _Wait(seconds, milliseconds, GSOCK_OUTPUT_FLAG);
+    return DoWait(seconds, milliseconds, GSOCK_OUTPUT_FLAG | GSOCK_LOST_FLAG);
 }
 
 bool wxSocketBase::WaitForLost(long seconds, long milliseconds)
 {
-    return _Wait(seconds, milliseconds, GSOCK_LOST_FLAG);
+    return DoWait(seconds, milliseconds, GSOCK_LOST_FLAG);
 }
 
 // --------------------------------------------------------------------------
@@ -1002,11 +982,6 @@ void LINKAGEMODE wx_socket_callback(GSocket * WXUNUSED(socket),
 
 void wxSocketBase::OnRequest(wxSocketNotify notification)
 {
-    // NOTE: We duplicate some of the code in _Wait, but this doesn't
-    //   hurt. It has to be here because the (GSocket) event might arrive
-    //   a bit delayed, and it has to be in _Wait as well because we don't
-    //   know whether the Wait functions are being used.
-
     switch(notification)
     {
         case wxSOCKET_CONNECTION:
@@ -1241,7 +1216,7 @@ wxSocketBase *wxSocketServer::Accept(bool wait)
 
 bool wxSocketServer::WaitForAccept(long seconds, long milliseconds)
 {
-    return _Wait(seconds, milliseconds, GSOCK_CONNECTION_FLAG);
+    return DoWait(seconds, milliseconds, GSOCK_CONNECTION_FLAG);
 }
 
 bool wxSocketBase::GetOption(int level, int optname, void *optval, int *optlen)
@@ -1411,14 +1386,22 @@ bool wxSocketClient::Connect(const wxSockAddress& addr_man,
 
 bool wxSocketClient::WaitOnConnect(long seconds, long milliseconds)
 {
-    if (m_connected)                      // Already connected
+    if ( m_connected )
+    {
+        // this happens if the initial attempt to connect succeeded without
+        // blocking
         return true;
+    }
 
-    if (!m_establishing || !m_socket)     // No connection in progress
+    wxCHECK_MSG( m_establishing && m_socket, false,
-        return false;
+                 "No connection establishment attempt in progress" );
 
-    return _Wait(seconds, milliseconds, GSOCK_CONNECTION_FLAG |
+    // we must specify GSOCK_LOST_FLAG here explicitly because we must return
-                                        GSOCK_LOST_FLAG);
+    // true if the connection establishment process is finished, whether it is
+    // over because we successfully connected or because we were not able to
+    // connect
+    return DoWait(seconds, milliseconds,
+        GSOCK_CONNECTION_FLAG | GSOCK_LOST_FLAG);
 }
 
 // ==========================================================================