thread safety issue about wxCommandEvent in wx2.8.12

If you are using the main C++ distribution of wxWidgets, Feel free to ask any question related to wxWidgets development here. This means questions regarding to C++ and wxWidgets, not compile problems.
Post Reply
ollydbg23
Super wx Problem Solver
Super wx Problem Solver
Posts: 438
Joined: Fri Dec 12, 2008 10:31 am

thread safety issue about wxCommandEvent in wx2.8.12

Post by ollydbg23 »

Hi, for some reasons, I would like send a wxCommandEvent from a working thread to the main Gui, here is the code snippet:

Code: Select all

wxThread::ExitCode MyThread::Entry()
{

    for (int n=0; n<1000; n++)
    {
        //this->Sleep(5);
        wxString str = wxT("hhhh");
        // notify the main thread
        wxCommandEvent event( wxEVT_COMMAND_TEXT_UPDATED, NUMBER_UPDATE_ID );
        event.SetInt(n);  
        if (n%2 == 0)
            str[0] = wxT('0');
        else
            str[0] = wxT('1');
        event.SetString(str.c_str());
        m_parent->GetEventHandler()->AddPendingEvent( event );
    }
    return 0;
}
Here, I have a "event" which type is wxCommandEvent, and by reading some posts in this forum, I use the statement:

Code: Select all

event.SetString(str.c_str());
Here, as wx2.8.12's wxString use reference counting, so to force a deep copy, I use the above line. I did a deep copy of string contents from "str" to "event.m_cmdString".
The code the GCC compiler generate here in-fact looks like below:

Code: Select all

construct a temp wxString say: tempStr from str.c_str();        //nRefs = 1
run the assignment operator function, so event.m_cmdString get assigned; //nRefs = 2
destroy the temp wxString, so nRefs return back to 1
Now, let's see the line:

Code: Select all

m_parent->GetEventHandler()->AddPendingEvent( event );
To make things simple, AddPendingEvent() function just make a clone of the event, and added the new cloned event to a event list. But note that this does not involve a deep copy of the wxString member of the event (m_cmdSting). After the statement above, under GDB, I have:
[debug]> p event.m_cmdString.GetStringData()->nRefs
[debug]$12 = 2
So, the wxString (event.m_cmdString) is still shared.

The nRefs value will return to 1 if "event" de-constructed.
But what happens if the OS did a thread switch(scheduled) before "event" de-constructed, there are two things going to happen:
1, "event" de-constructed in the worker thread
2, the new cloned event get processed in the main GUI thread
I don't know what happens first, maybe, they will get concurrently running. So, this is a kind of thread safety issue? Right?

Finally, I think the only way is: we did a full deep copy of every thing in the AddPendingEvent() function? Looks like there is not such way in wx2.8.12.
In wx.2.9.x, we can use wxThreadEvent Class?
User avatar
doublemax
Moderator
Moderator
Posts: 19115
Joined: Fri Apr 21, 2006 8:03 pm
Location: $FCE2

Re: thread safety issue about wxCommandEvent in wx2.8.12

Post by doublemax »

Code: Select all

construct a temp wxString say: tempStr from str.c_str();        //nRefs = 1
run the assignment operator function, so event.m_cmdString get assigned; //nRefs = 2
That shouldn't happen. str.c_str() is supposed to return a "const wxChar *", so a new wxString should be created like from a string literal. The actual string data should not be shared.

Try an explicit cast of str.c_str() to "const wxChar *"
Use the source, Luke!
ollydbg23
Super wx Problem Solver
Super wx Problem Solver
Posts: 438
Joined: Fri Dec 12, 2008 10:31 am

Re: thread safety issue about wxCommandEvent in wx2.8.12

Post by ollydbg23 »

doublemax wrote:

Code: Select all

construct a temp wxString say: tempStr from str.c_str();        //nRefs = 1
run the assignment operator function, so event.m_cmdString get assigned; //nRefs = 2
That shouldn't happen. str.c_str() is supposed to return a "const wxChar *", so a new wxString should be created like from a string literal. The actual string data should not be shared.

Try an explicit cast of str.c_str() to "const wxChar *"
Ok, this is not the core issue about my first post, because the result wxStrings, one is event.m_cmdString and the other is str is NOT shared.

The biggest problem is that event.m_cmdString was shared with a new cloned event's m_cmdString.
User avatar
doublemax
Moderator
Moderator
Posts: 19115
Joined: Fri Apr 21, 2006 8:03 pm
Location: $FCE2

Re: thread safety issue about wxCommandEvent in wx2.8.12

Post by doublemax »

The biggest problem is that event.m_cmdString was shared with a new cloned event's m_cmdString.
a) I still don't see how this is possible.
b) It doesn't really matter because the first wxString instance will never be touched again
c) If you want to be 100% sure, just derive your own class from wxCommandEvent and override the Clone() method to ensure a deep string copy.
http://forums.wxwidgets.org/viewtopic.p ... 03#p103503
Use the source, Luke!
ollydbg23
Super wx Problem Solver
Super wx Problem Solver
Posts: 438
Joined: Fri Dec 12, 2008 10:31 am

Re: thread safety issue about wxCommandEvent in wx2.8.12

Post by ollydbg23 »

doublemax wrote:
The biggest problem is that event.m_cmdString was shared with a new cloned event's m_cmdString.
a) I still don't see how this is possible.
b) It doesn't really matter because the first wxString instance will never be touched again
Let me explain:
We would like to send a wxString(named str in my case) from worker thread to Main GUI thread, this string is carried by wxCommandEvent. First, it use

Code: Select all

event.SetString(str.c_str());
We can pretty sure that event.m_cmdString has a deep copy of the str.
Now, after the function:

Code: Select all

m_parent->GetEventHandler()->AddPendingEvent( event );
event.m_cmdString share the internal buffer with cloned new_event.m_cmdString
event.m_cmdString is going to die because event is a local variable, it is going to destructed in the worker thread
new_event.m_cmdString is going to die because new_event will be deleted in the main GUI thread after all the event handler is called.


Pseudocode of wxString's Destructor:

Code: Select all

--nRefs;
if (nRefs==0) 
    delete the internal buffer

In wx2.8.12's wxString implementation, reference counter(nRefs) is not a atomic variable, We can imagine the --nRefs are some code like:

Code: Select all

read nRefs from memory to CPU register
decrease the CPU register by one
write back to nRefs in memory
So, we can have such interlaced execution of the code

Code: Select all

Worker thread  : read event.m_cmdString nRefs value 2 from memory to CPU register
OS Schedule    : swith to Main GUI
Main GUI thread: read new_event.m_cmdString nRefs value 2 from memory to CPU register
Main GUI thread: decrease CPU register 
Main GUI thread: write back to nRefs which is 1
Main GUI thread: if (nRefs==0) did not match, so internal buffer not deleted
OS Schedule    : swith to worker thread
Worker thread  : decrease CPU register (value is 1)
Worker thread  : write back to nRefs which is 1
Worker thread  : if (nRefs==0) did not match, so internal buffer not deleted
Now, finally the "internal buffer" is leaked after the two desturctor of wxString is called. Note this is a simplified example, I think there are many cases that operations on nRefs are not atomic. Hope you can understand my concern.
c) If you want to be 100% sure, just derive your own class from wxCommandEvent and override the Clone() method to ensure a deep string copy.
http://forums.wxwidgets.org/viewtopic.p ... 03#p103503
Thanks, I think override Clone() of the new event class is the 100% safe way.
ollydbg23
Super wx Problem Solver
Super wx Problem Solver
Posts: 438
Joined: Fri Dec 12, 2008 10:31 am

Re: thread safety issue about wxCommandEvent in wx2.8.12

Post by ollydbg23 »

doublemax wrote:
The biggest problem is that event.m_cmdString was shared with a new cloned event's m_cmdString.
a) I still don't see how this is possible.
b) It doesn't really matter because the first wxString instance will never be touched again
More explanation about this:
AddPendingEvent( event ) function will internally clone the event, but wxCommandEvent's Clone() function does not do a deep copy of it's m_cmdString member, see below:

Code: Select all

virtual wxEvent *Clone() const { return new wxCommandEvent(*this); }

Code: Select all

    wxCommandEvent(const wxCommandEvent& event)
        : wxEvent(event),
#if WXWIN_COMPATIBILITY_2_4
          m_commandString(this),
#endif
          m_cmdString(event.m_cmdString),
          m_commandInt(event.m_commandInt),
          m_extraLong(event.m_extraLong),
          m_clientData(event.m_clientData),
          m_clientObject(event.m_clientObject)
        { }
The bad thing is here:

Code: Select all

m_cmdString(event.m_cmdString)
A wxString is construct from another wxString by using copy constructor, so reference counting is used here.
User avatar
doublemax
Moderator
Moderator
Posts: 19115
Joined: Fri Apr 21, 2006 8:03 pm
Location: $FCE2

Re: thread safety issue about wxCommandEvent in wx2.8.12

Post by doublemax »

I see it now, thanks for the explanation.

In wx 2.9.x wxString has a new method Clone() for this, which is used in wxThreadEvent:

Code: Select all

wxThreadEvent(const wxThreadEvent& event)
    : wxEvent(event),
      wxEventAnyPayloadMixin(event)
{
    // make sure our string member (which uses COW, aka refcounting) is not
    // shared by other wxString instances:
    SetString(GetString().Clone());
}
Use the source, Luke!
Post Reply