From c3f941621e05420c0d81d7759321857e732eb8f7 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Mon, 28 Apr 2008 18:49:42 +0000 Subject: [PATCH] added wxQueueEvent() avoiding the bug of wxPostEvent() with the events having wxString fields git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@53405 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775 --- docs/changes.txt | 2 + include/wx/event.h | 45 +++++++++++++++--- interface/event.h | 106 +++++++++++++++++++++++++++++++++---------- src/common/event.cpp | 14 ++---- 4 files changed, 127 insertions(+), 40 deletions(-) diff --git a/docs/changes.txt b/docs/changes.txt index 5a21ed1904..e7ce5694bf 100644 --- a/docs/changes.txt +++ b/docs/changes.txt @@ -237,6 +237,8 @@ All: - Added wxWeakRef, wxScopedPtr, wxSharedPtr class templates - Added wxVector class templates - Added wxON_BLOCK_EXIT_SET() and wxON_BLOCK_EXIT_NULL() to wx/scopeguard.h. +- Added wxEvtHandler::QueueEvent() replacing AddPendingEvent() and + wxQueueEvent() replacing wxPostEvent(). All (Unix): diff --git a/include/wx/event.h b/include/wx/event.h index 7de2c88a81..e9b0220996 100644 --- a/include/wx/event.h +++ b/include/wx/event.h @@ -439,7 +439,7 @@ public: wxCommandEvent(const wxCommandEvent& event) : wxEvent(event), - m_cmdString(event.m_cmdString.c_str()), // "thread-safe" + m_cmdString(event.m_cmdString), m_commandInt(event.m_commandInt), m_extraLong(event.m_extraLong), m_clientData(event.m_clientData), @@ -2276,7 +2276,9 @@ public: void SetEvtHandlerEnabled(bool enabled) { m_enabled = enabled; } bool GetEvtHandlerEnabled() const { return m_enabled; } - // process an event right now + // Process an event right now: this can only be called from the main + // thread, use QueueEvent() for scheduling the events for + // processing from other threads. virtual bool ProcessEvent(wxEvent& event); // Process an event by calling ProcessEvent and handling any exceptions @@ -2285,8 +2287,25 @@ public: // wouldn't correctly propagate to wxEventLoop. bool SafelyProcessEvent(wxEvent& event); - // add an event to be processed later - virtual void AddPendingEvent(const wxEvent& event); + // Schedule the given event to be processed later. It takes ownership of + // the event pointer, i.e. it will be deleted later. This is safe to call + // from multiple threads although you still need to ensure that wxString + // fields of the event object are deep copies and not use the same string + // buffer as other wxString objects in this thread. + virtual void QueueEvent(wxEvent *event); + + // Add an event to be processed later: notice that this function is not + // safe to call from threads other than main, use QueueEvent() + virtual void AddPendingEvent(const wxEvent& event) + { + // notice that the thread-safety problem comes from the fact that + // Clone() doesn't make deep copies of wxString fields of wxEvent + // object and so the same wxString could be used from both threads when + // the event object is destroyed in this one -- QueueEvent() avoids + // this problem as the event pointer is not used any more in this + // thread at all after it is called. + QueueEvent(event.Clone()); + } void ProcessPendingEvents(); @@ -2491,15 +2510,27 @@ private: }; #endif // wxUSE_WEAKREF -// Post a message to the given eventhandler which will be processed during the -// next event loop iteration +// Post a message to the given event handler which will be processed during the +// next event loop iteration. +// +// Notice that this one is not thread-safe, use wxQueueEvent() inline void wxPostEvent(wxEvtHandler *dest, const wxEvent& event) { - wxCHECK_RET( dest, wxT("need an object to post event to in wxPostEvent") ); + wxCHECK_RET( dest, "need an object to post event to" ); dest->AddPendingEvent(event); } +// Wrapper around wxEvtHandler::QueueEvent(): adds an event for later +// processing, unlike wxPostEvent it is safe to use from different thread even +// for events with wxString members +inline void wxQueueEvent(wxEvtHandler *dest, wxEvent *event) +{ + wxCHECK_RET( dest, "need an object to queue event for" ); + + dest->QueueEvent(event); +} + typedef void (wxEvtHandler::*wxEventFunction)(wxEvent&); typedef void (wxEvtHandler::*wxIdleEventFunction)(wxIdleEvent&); diff --git a/interface/event.h b/interface/event.h index 81ff87735c..1b74689488 100644 --- a/interface/event.h +++ b/interface/event.h @@ -42,8 +42,9 @@ public: /** Returns a copy of the event. - Any event that is posted to the wxWidgets event system for later action (via - wxEvtHandler::AddPendingEvent or wxPostEvent()) must implement this method. + Any event that is posted to the wxWidgets event system for later action + (via wxEvtHandler::AddPendingEvent or wxPostEvent()) must implement + this method. All wxWidgets events fully implement this method, but any derived events implemented by the user should also implement this method just in case they @@ -265,31 +266,72 @@ public: virtual ~wxEvtHandler(); /** - This function posts an event to be processed later. + Queue event for a later processing. + + This method is similar to ProcessEvent() but while the latter is + synchronous, i.e. the event is processed immediately, before the + function returns, this one is asynchronous and returns immediately + while the event will be processed at some later time (usually during + the next event loop iteration). + + Another important difference is that this method takes ownership of the + @a event parameter, i.e. it will delete it itself. This implies that + the event should be allocated on the heap and that the pointer can't be + used any more after the function returns (as it can be deleted at any + moment). + + QueueEvent() can be used for inter-thread communication from the worker + threads to the main thread, it is safe in the sense that it uses + locking internally and avoids the problem mentioned in AddPendingEvent() + documentation by ensuring that the @a event object is not used by the + calling thread any more. Care should still be taken to avoid that some + fields of this object are used by it, notably any wxString members of + the event object must not be shallow copies of another wxString object + as this would result in them still using the same string buffer behind + the scenes. For example + @code + void FunctionInAWorkerThread(const wxString& str) + { + wxCommandEvent * const e = new wxCommandEvent; - The difference between sending an event (using the ProcessEvent - method) and posting it is that in the first case the event is - processed before the function returns, while in the second case, - the function returns immediately and the event will be processed - sometime later (usually during the next event loop iteration). + // NOT e->SetString(str) as this would be a shallow copy + e->SetString(str.c_str()); // make a deep copy - A copy of event is made by the function, so the original can be deleted as - soon as function returns (it is common that the original is created on the - stack). This requires that the wxEvent::Clone method be implemented by event - so that it can be duplicated and stored until it gets processed. + wxTheApp->QueueEvent(new wxCommandEvent + } + @endcode - This is also the method to call for inter-thread communication - it will post - events safely between different threads which means that this method is - thread-safe by using critical sections where needed. In a multi-threaded program, - you often need to inform the main GUI thread about the status of other working - threads and such notification should be done using this method. + Finally notice that this method automatically wakes up the event loop + if it is currently idle by calling ::wxWakeUpIdle() so there is no need + to do it manually when using it. - This method automatically wakes up idle handling if the underlying window - system is currently idle and thus would not send any idle events. - (Waking up idle handling is done calling ::wxWakeUpIdle.) + @since 2.9.0 @param event - Event to add to process queue. + A heap-allocated event to be queued, QueueEvent() takes ownership + of it. This parameter shouldn't be @c NULL. + */ + virtual void QueueEvent(wxEvent *event); + + /** + Post an event to be processed later. + + This function is similar to QueueEvent() but can't be used to post + events from worker threads for the event objects with wxString fields + (i.e. in practice most of them) because of an unsafe use of the same + wxString object which happens because the wxString field in the + original @a event object and its copy made internally by this function + share the same string buffer internally. Use QueueEvent() to avoid + this. + + A copy of event is made by the function, so the original can be deleted + as soon as function returns (it is common that the original is created + on the stack). This requires that the wxEvent::Clone() method be + implemented by event so that it can be duplicated and stored until it + gets processed. + + @param event + Event to add to the pending events queue. */ virtual void AddPendingEvent(const wxEvent& event); @@ -3264,11 +3306,29 @@ public: Otherwise, it dispatches @a event immediately using wxEvtHandler::ProcessEvent(). See the respective documentation for details - (and caveats). + (and caveats). Because of limitation of wxEvtHandler::AddPendingEvent() + this function is not thread-safe for event objects having wxString fields, + use wxQueueEvent() instead. @header{wx/event.h} */ -void wxPostEvent(wxEvtHandler* dest, wxEvent& event); +void wxPostEvent(wxEvtHandler* dest, const wxEvent& event); + +/** + Queue an event for processing on the given object. + + This is a wrapper around wxEvtHandler::QueueEvent(), see its documentation + for more details. + + @header{wx/event.h} + + @param dest + The object to queue the event on, can't be @c NULL. + @param event + The heap-allocated and non-@c NULL event to queue, the function takes + ownership of it. + */ +void wxQueueEvent(wxEvtHandler* dest, wxEvent *event); //@} diff --git a/src/common/event.cpp b/src/common/event.cpp index 817f40e4a8..d2bd83d29b 100644 --- a/src/common/event.cpp +++ b/src/common/event.cpp @@ -1130,23 +1130,17 @@ bool wxEvtHandler::ProcessThreadEvent(const wxEvent& event) #endif // wxUSE_THREADS -void wxEvtHandler::AddPendingEvent(const wxEvent& event) +void wxEvtHandler::QueueEvent(wxEvent *event) { - // 1) Add event to list of pending events of this event handler - - wxEvent *eventCopy = event.Clone(); - - // we must be able to copy the events here so the event class must - // implement Clone() properly instead of just providing a NULL stab for it - wxCHECK_RET( eventCopy, - _T("events of this type aren't supposed to be posted") ); + wxCHECK_RET( event, "NULL event can't be posted" ); + // 1) Add this event to our list of pending events wxENTER_CRIT_SECT( m_pendingEventsLock ); if ( !m_pendingEvents ) m_pendingEvents = new wxList; - m_pendingEvents->Append(eventCopy); + m_pendingEvents->Append(event); wxLEAVE_CRIT_SECT( m_pendingEventsLock ); -- 2.45.2