From e0102c2396df94c574a331db0443613eca748c51 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 20 Nov 2019 18:47:09 +0100 Subject: [PATCH 1/3] Allow disabling events for blocking sockets in Unix version It should still be possible to use DoEnableEvents() to disable events for blocking sockets and this can be useful if a previously non-blocking socket became blocking due to a SetFlags(wxSOCKET_BLOCK) call, so adjust the fix of e18c8fd29a10b1b87ea5cff7c5b3a16fe5b32690 (see #17031) to avoid calling EnableEvents() for blocking sockets instead of ignoring them in DoEnableEvents() itself. Also add an assert checking that we never try enabling events for blocking sockets as this still doesn't make sense and so shouldn't happen. No real changes yet, but this is necessary for the upcoming commits. See #12886. --- include/wx/unix/private/sockunix.h | 4 ++++ src/unix/sockunix.cpp | 11 ++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/include/wx/unix/private/sockunix.h b/include/wx/unix/private/sockunix.h index 05d043d354..2e4506bcbc 100644 --- a/include/wx/unix/private/sockunix.h +++ b/include/wx/unix/private/sockunix.h @@ -40,6 +40,10 @@ public: virtual void ReenableEvents(wxSocketEventFlags flags) wxOVERRIDE { + // Events are only ever used for non-blocking sockets. + if ( GetSocketFlags() & wxSOCKET_BLOCK ) + return; + // enable the notifications about input/output being available again in // case they were disabled by OnRead/WriteWaiting() // diff --git a/src/unix/sockunix.cpp b/src/unix/sockunix.cpp index 78542648ef..e1ffee352a 100644 --- a/src/unix/sockunix.cpp +++ b/src/unix/sockunix.cpp @@ -90,17 +90,18 @@ wxSocketError wxSocketImplUnix::GetLastError() const void wxSocketImplUnix::DoEnableEvents(int flags, bool enable) { - // No events for blocking sockets, they should be usable from the other - // threads and the events only work for the sockets used by the main one. - if ( GetSocketFlags() & wxSOCKET_BLOCK ) - return; - wxSocketManager * const manager = wxSocketManager::Get(); if (!manager) return; if ( enable ) { + // We should never try to enable events for the blocking sockets, they + // should be usable from the other threads and the events only work for + // the sockets used by the main one. + wxASSERT_MSG( !(GetSocketFlags() & wxSOCKET_BLOCK), + "enabling events for a blocking socket?" ); + if ( flags & wxSOCKET_INPUT_FLAG ) manager->Install_Callback(this, wxSOCKET_INPUT); if ( flags & wxSOCKET_OUTPUT_FLAG ) From 51ea713826bffc0db0b7d6ec3ad39a8ea6f947a3 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 20 Nov 2019 18:55:51 +0100 Subject: [PATCH 2/3] Extend and rename wxSocketImpl::UnblockAndRegisterWithEventLoop() In addition to unblocking and registering the socket, also support using this function to make the socket blocking and unregistering it from the event loop, if its flags include wxSOCKET_BLOCK. This was already half-done by wxMSW, which took wxSOCKET_BLOCK presence into account in its implementation, but not by the Unix implementation. Now do it under all platforms, as this will be useful for switching a previously non-blocking socket to blocking mode. Finally, rename the function to better reflect what it really does. See #12886. --- include/wx/msw/private/sockmsw.h | 5 ++++- include/wx/private/socket.h | 10 ++++++---- include/wx/unix/private/sockunix.h | 9 +++++---- src/common/socket.cpp | 6 +++--- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/include/wx/msw/private/sockmsw.h b/include/wx/msw/private/sockmsw.h index b79aa8e094..70844e4742 100644 --- a/include/wx/msw/private/sockmsw.h +++ b/include/wx/msw/private/sockmsw.h @@ -60,7 +60,7 @@ public: private: virtual void DoClose() wxOVERRIDE; - virtual void UnblockAndRegisterWithEventLoop() wxOVERRIDE + virtual void UpdateBlockingState() wxOVERRIDE { if ( GetSocketFlags() & wxSOCKET_BLOCK ) { @@ -74,6 +74,9 @@ private: // would result in data races and other unpleasantness. wxIoctlSocketArg_t trueArg = 1; ioctlsocket(m_fd, FIONBIO, &trueArg); + + // Uninstall it in case it was installed before. + wxSocketManager::Get()->Uninstall_Callback(this); } else { diff --git a/include/wx/private/socket.h b/include/wx/private/socket.h index fe3d3b2df0..2d184acc07 100644 --- a/include/wx/private/socket.h +++ b/include/wx/private/socket.h @@ -323,9 +323,11 @@ private: // called by Close() if we have a valid m_fd virtual void DoClose() = 0; - // put this socket into non-blocking mode and enable monitoring this socket - // as part of the event loop - virtual void UnblockAndRegisterWithEventLoop() = 0; + // Update the socket depending on the presence or absence of wxSOCKET_BLOCK + // in GetSocketFlags(): if it's present, make the socket blocking and + // ensure that we don't get any asynchronous event for it, otherwise put + // it into non-blocking mode and enable monitoring it in the event loop. + virtual void UpdateBlockingState() = 0; // check that the socket wasn't created yet and that the given address // (either m_local or m_peer depending on the socket kind) is valid and @@ -350,7 +352,7 @@ private: } // apply the options to the (just created) socket and register it with the - // event loop by calling UnblockAndRegisterWithEventLoop() + // event loop by calling UpdateBlockingState() void PostCreation(); // update local address after binding/connecting diff --git a/include/wx/unix/private/sockunix.h b/include/wx/unix/private/sockunix.h index 2e4506bcbc..8b3fc2bbb0 100644 --- a/include/wx/unix/private/sockunix.h +++ b/include/wx/unix/private/sockunix.h @@ -73,12 +73,13 @@ private: wxCloseSocket(m_fd); } - virtual void UnblockAndRegisterWithEventLoop() wxOVERRIDE + virtual void UpdateBlockingState() wxOVERRIDE { - int trueArg = 1; - ioctl(m_fd, FIONBIO, &trueArg); + // Make this int and not bool to allow passing it to ioctl(). + const int isBlocking = (GetSocketFlags() & wxSOCKET_BLOCK) != 0; + ioctl(m_fd, FIONBIO, &isBlocking); - EnableEvents(); + DoEnableEvents(wxSOCKET_INPUT_FLAG | wxSOCKET_OUTPUT_FLAG, !isBlocking); } // enable or disable notifications for socket input/output events diff --git a/src/common/socket.cpp b/src/common/socket.cpp index 23276fb412..3ae69fadd1 100644 --- a/src/common/socket.cpp +++ b/src/common/socket.cpp @@ -366,9 +366,9 @@ void wxSocketImpl::PostCreation() if ( m_initialSendBufferSize >= 0 ) SetSocketOption(SO_SNDBUF, m_initialSendBufferSize); - // we always put our sockets in unblocked mode and handle blocking + // Call this to put our socket in unblocked mode: we'll handle blocking // ourselves in DoRead/Write() if wxSOCKET_WAITALL is specified - UnblockAndRegisterWithEventLoop(); + UpdateBlockingState(); } wxSocketError wxSocketImpl::UpdateLocalAddress() @@ -551,7 +551,7 @@ wxSocketImpl *wxSocketImpl::Accept(wxSocketBase& wxsocket) sock->m_fd = fd; sock->m_peer = wxSockAddressImpl(from.addr, fromlen); - sock->UnblockAndRegisterWithEventLoop(); + sock->UpdateBlockingState(); return sock; } From 73f0c9dff8b84f22f799d2fb68f793eef4fc36bc Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Wed, 20 Nov 2019 19:00:30 +0100 Subject: [PATCH 3/3] Fix crash with blocking accepting sockets in threads under Unix Sockets returned by wxSocket::Accept() are non-blocking by default and the only way to use them safely in worker threads is by switching them to the blocking mode by calling SetFlags(wxSOCKET_BLOCK). However this didn't work correctly since at least 2.8 days, as turning wxSOCKET_BLOCK on didn't unregister the socket from the event loop, with which it had been registered on creation. Fix this by doing this now, which ensures that the main thread doesn't get any notifications about the socket if it's used, in a blocking way, in a worker thread. Note that making the new socket blocking after accpeting is still pretty inefficient and pre-creating the socket as blocking and using AcceptWith() is still preferable, but at least it does work now. Closes #12886. --- include/wx/msw/private/sockmsw.h | 6 +++--- include/wx/private/socket.h | 12 ++++++------ include/wx/unix/private/sockunix.h | 18 +++++++++--------- src/common/socket.cpp | 13 +++++++++++++ 4 files changed, 31 insertions(+), 18 deletions(-) diff --git a/include/wx/msw/private/sockmsw.h b/include/wx/msw/private/sockmsw.h index 70844e4742..d6010fbc1b 100644 --- a/include/wx/msw/private/sockmsw.h +++ b/include/wx/msw/private/sockmsw.h @@ -57,9 +57,6 @@ public: // anything here } -private: - virtual void DoClose() wxOVERRIDE; - virtual void UpdateBlockingState() wxOVERRIDE { if ( GetSocketFlags() & wxSOCKET_BLOCK ) @@ -86,6 +83,9 @@ private: } } +private: + virtual void DoClose() wxOVERRIDE; + int m_msgnumber; friend class wxSocketMSWManager; diff --git a/include/wx/private/socket.h b/include/wx/private/socket.h index 2d184acc07..20db91c7e7 100644 --- a/include/wx/private/socket.h +++ b/include/wx/private/socket.h @@ -281,6 +281,12 @@ public: // notifications // ------------- + // Update the socket depending on the presence or absence of wxSOCKET_BLOCK + // in GetSocketFlags(): if it's present, make the socket blocking and + // ensure that we don't get any asynchronous event for it, otherwise put + // it into non-blocking mode and enable monitoring it in the event loop. + virtual void UpdateBlockingState() = 0; + // notify m_wxsocket about the given socket event by calling its (inaptly // named) OnRequest() method void NotifyOnStateChange(wxSocketNotify event); @@ -323,12 +329,6 @@ private: // called by Close() if we have a valid m_fd virtual void DoClose() = 0; - // Update the socket depending on the presence or absence of wxSOCKET_BLOCK - // in GetSocketFlags(): if it's present, make the socket blocking and - // ensure that we don't get any asynchronous event for it, otherwise put - // it into non-blocking mode and enable monitoring it in the event loop. - virtual void UpdateBlockingState() = 0; - // check that the socket wasn't created yet and that the given address // (either m_local or m_peer depending on the socket kind) is valid and // set m_error and return false if this is not the case diff --git a/include/wx/unix/private/sockunix.h b/include/wx/unix/private/sockunix.h index 8b3fc2bbb0..f07ecafd87 100644 --- a/include/wx/unix/private/sockunix.h +++ b/include/wx/unix/private/sockunix.h @@ -59,6 +59,15 @@ public: EnableEvents(flags); } + virtual void UpdateBlockingState() wxOVERRIDE + { + // Make this int and not bool to allow passing it to ioctl(). + const int isBlocking = (GetSocketFlags() & wxSOCKET_BLOCK) != 0; + ioctl(m_fd, FIONBIO, &isBlocking); + + DoEnableEvents(wxSOCKET_INPUT_FLAG | wxSOCKET_OUTPUT_FLAG, !isBlocking); + } + // wxFDIOHandler methods virtual void OnReadWaiting() wxOVERRIDE; virtual void OnWriteWaiting() wxOVERRIDE; @@ -73,15 +82,6 @@ private: wxCloseSocket(m_fd); } - virtual void UpdateBlockingState() wxOVERRIDE - { - // Make this int and not bool to allow passing it to ioctl(). - const int isBlocking = (GetSocketFlags() & wxSOCKET_BLOCK) != 0; - ioctl(m_fd, FIONBIO, &isBlocking); - - DoEnableEvents(wxSOCKET_INPUT_FLAG | wxSOCKET_OUTPUT_FLAG, !isBlocking); - } - // enable or disable notifications for socket input/output events void EnableEvents(int flags = wxSOCKET_INPUT_FLAG | wxSOCKET_OUTPUT_FLAG) { DoEnableEvents(flags, true); } diff --git a/src/common/socket.cpp b/src/common/socket.cpp index 3ae69fadd1..f6a5f57fa6 100644 --- a/src/common/socket.cpp +++ b/src/common/socket.cpp @@ -1672,7 +1672,20 @@ void wxSocketBase::SetFlags(wxSocketFlags flags) "Using wxSOCKET_WAITALL or wxSOCKET_BLOCK with " "wxSOCKET_NOWAIT doesn't make sense" ); + // Blocking sockets are very different from non-blocking ones and we need + // to [un]register the socket with the event loop if wxSOCKET_BLOCK is + // being [un]set. + const bool + blockChanged = (m_flags & wxSOCKET_BLOCK) != (flags & wxSOCKET_BLOCK); + m_flags = flags; + + if ( blockChanged ) + { + // Of course, we only do this if we already have the actual socket. + if ( m_impl ) + m_impl->UpdateBlockingState(); + } }