wxString objects are not Thread Safe

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.
maxderhak
Earned a small fee
Earned a small fee
Posts: 11
Joined: Fri Apr 13, 2007 6:02 pm

wxString objects are not Thread Safe

Post by maxderhak »

I really like wxStrings. They provide a lot more functionality over std::string objects. We tried to use wsStrings in situations where multiple threads made read access to a single wxString. The result was catastrophe. Apparently the wxString keeps a local copy of a multibyte version of the string that would get overridden by the multiple threads.

Is there any thought to making wxString objects more thread safe?
The Max
megabyte
I live to help wx-kind
I live to help wx-kind
Posts: 196
Joined: Tue Dec 07, 2004 8:54 pm
Location: Essen, Germany

Post by megabyte »

wxWidgets has a number of synchronization primitives like wxMutex, wxCriticalSection, wxCondition. Use them to organize an access to the string. It can looks like

Code: Select all

static wxCriticalSection MyClass::m_csStringAccess;
...
void MyClass::WriteString(const wxString &str) {
        wxCriticalSectionLocker lock(m_csStringAccess);
        m_strValue = str.c_str(); // necessary for memory copy instead of reference counting
}
maxderhak
Earned a small fee
Earned a small fee
Posts: 11
Joined: Fri Apr 13, 2007 6:02 pm

Post by maxderhak »

This is essentially what we had to do. The fact that we had to do it was a big blow to the desire of several of our engineers in using wxWidgets. In fact a lot of the development using wxWidgets was stopped because of this. It took weeks to undo lots of development.

It would be really nice if the read accessors of a wxString handled the syncronization on thier own. I don't mind having to do syncronization on writes, but the object could be smart on its own related to reads.
The Max
megabyte
I live to help wx-kind
I live to help wx-kind
Posts: 196
Joined: Tue Dec 07, 2004 8:54 pm
Location: Essen, Germany

Post by megabyte »

How do you imagine a read synchronization for the following code

Code: Select all

...
cont wxChar *ptr = str.c_str();
....
printf(_T("%s\n"), ptr);
...
I think it should be something like that

Code: Select all

...
{
  wxCriticalSectionLocker lock(str.GetAccessCS());
  cont wxChar *ptr = str.c_str();
  ....
  printf(_T("%s\n"), ptr);
}
...
But this code has a lack. The GetAccessCS cannot be a const method and is not be able to be used everythere. So I think is easier to organize thread safe access out of a string class.
maxderhak
Earned a small fee
Earned a small fee
Posts: 11
Joined: Fri Apr 13, 2007 6:02 pm

Post by maxderhak »

Actually I just want to have the following.

printf(_T("%s\n"), str.c_str());

The c_str() member is essentially a read accessor. It doesn't change the essential contents of the wxString object. (I.E. The value of of str should be the same before and after the call). Why should I have to syncronize to use it?
The Max
lowjoel
Part Of The Furniture
Part Of The Furniture
Posts: 1511
Joined: Sun Jun 19, 2005 11:37 am
Location: Singapore
Contact:

Post by lowjoel »

Because as thread A reads from the string using c_str(), thread B may be writing to the string. Granted c_str is a const-member function, thread B changed the data as A is reading from it, which basically will result in undefined behaviour.

Joel
megabyte
I live to help wx-kind
I live to help wx-kind
Posts: 196
Joined: Tue Dec 07, 2004 8:54 pm
Location: Essen, Germany

Post by megabyte »

maxderhak wrote:Actually I just want to have the following.

printf(_T("%s\n"), str.c_str());

The c_str() member is essentially a read accessor. It doesn't change the essential contents of the wxString object. (I.E. The value of of str should be the same before and after the call). Why should I have to syncronize to use it?
Ok. For instance, c_str() has access protection like this

Code: Select all

class wxString {
...
  mutable wxCriticalSection m_csAccess;
...
};

...

const wxChar *wxString::c_str() const {
  wxCriticalSectionLocker lock(m_csAccess);
  ...
  return m_pData;
}
The locker locks other read and write methods only during c_str execution.

There is a dummy code

Code: Select all

cont wxChar *ptr = str.c_str();
wxMilliSleep(100);
printf(_T("%s\n"), ptr);
The string content before calling c_str is XXXXXXXXXXX. After the tread sleeping, printf starts to output the string characters. In the middle of output other thread changes XXXXXXXXXXX to YYYYYYYYYY. And the output is XXXXXYYYYY.
lowjoel
Part Of The Furniture
Part Of The Furniture
Posts: 1511
Joined: Sun Jun 19, 2005 11:37 am
Location: Singapore
Contact:

Post by lowjoel »

Read the STL documentation for c_str(). http://dinkumware.com/manuals/?manual=c ... ing::c_str
The member function returns a pointer to a non-modifiable C string constructed by adding a terminating null element (value_type()) to the controlled sequence. Calling any non-const member function for *this can invalidate the pointer.
You should NEVER assume that the pointer returned by c_str() will remain constant even if no modifications are made (the VC STL is strict about this.). Though the Dinkum STL states that, actually it is common knowledge through experience that c_str() pointers are very dangerous and should be used atomically (e.g by immediately strcpy'ing it out)

Joel
maxderhak
Earned a small fee
Earned a small fee
Posts: 11
Joined: Fri Apr 13, 2007 6:02 pm

Post by maxderhak »

lowjoel wrote:Because as thread A reads from the string using c_str(), thread B may be writing to the string. Granted c_str is a const-member function, thread B changed the data as A is reading from it, which basically will result in undefined behaviour.

Joel
No! This is not the case. Thread A initializes a string. Then threads B, C, D & E all potentially make calls to get the value of the string by calling c_str(). Once the string was initialized it is not modified again. It is just accessed. The problem is that the internal implementatin of wxString's c_str() causes a write to an internal private member to get the return value. I would like it if the syncronization to intialize and get the return value were put into the wxString object itself, so that outside users of the wxString wouldn't have to work around the internal implementation details of an operation that is essentially a read operation. Why should I syncronize when I know that the string is not being modified by another thread. The internal modification of prvate data that doesn't essentially change the state of the object is the problem.
The Max
lowjoel
Part Of The Furniture
Part Of The Furniture
Posts: 1511
Joined: Sun Jun 19, 2005 11:37 am
Location: Singapore
Contact:

Post by lowjoel »

Hm. Then it shows a flaw in wxString's implementation. But I doubt that's possible as c_str is a const member function, and I doubt there are mutable member variables.

Then again, you may not be using wx's wxString, as you may have defined wxUSE_STL to 1 and thus use std::string as the "main" string class and using wxString as a wrapper.

Joel
maxderhak
Earned a small fee
Earned a small fee
Posts: 11
Joined: Fri Apr 13, 2007 6:02 pm

Post by maxderhak »

lowjoel wrote:Hm. Then it shows a flaw in wxString's implementation. But I doubt that's possible as c_str is a const member function, and I doubt there are mutable member variables.

Then again, you may not be using wx's wxString, as you may have defined wxUSE_STL to 1 and thus use std::string as the "main" string class and using wxString as a wrapper.

Joel
We were using wx's wxString.
The Max
megabyte
I live to help wx-kind
I live to help wx-kind
Posts: 196
Joined: Tue Dec 07, 2004 8:54 pm
Location: Essen, Germany

Post by megabyte »

c_str does not modify any internal variables

Code: Select all

wxChar *m_pchData;
...
const wxChar* c_str() const { return m_pchData; }
lowjoel
Part Of The Furniture
Part Of The Furniture
Posts: 1511
Joined: Sun Jun 19, 2005 11:37 am
Location: Singapore
Contact:

Post by lowjoel »

That was what I was thinking, since c_str would then be unable to be a const member function. But if anecdotal evidence proves it wrong... I have no say lol.

Joel
maxderhak
Earned a small fee
Earned a small fee
Posts: 11
Joined: Fri Apr 13, 2007 6:02 pm

Post by maxderhak »

megabyte wrote:c_str does not modify any internal variables

Code: Select all

wxChar *m_pchData;
...
const wxChar* c_str() const { return m_pchData; }
Your right. The problem is with mb_str() or casting the wxString to (const char*). The scenario is the same.
The Max
megabyte
I live to help wx-kind
I live to help wx-kind
Posts: 196
Joined: Tue Dec 07, 2004 8:54 pm
Location: Essen, Germany

Post by megabyte »

maxderhak wrote:
megabyte wrote:c_str does not modify any internal variables

Code: Select all

wxChar *m_pchData;
...
const wxChar* c_str() const { return m_pchData; }
Your right. The problem is with mb_str() or casting the wxString to (const char*). The scenario is the same.
mb_str does not modify wxString. It creates an instance of the wxCharBuffer class. Also wxString has no operator const char *(), but only const wxChar *() which is similar to c_str(). The wxCharBuffer class has the const char *() operator.

Do you really need to convert wxString to const char *?

Edit: mb_str returns a local copy of wxCharBuffer, so I think the readers threads cannot create a lot of problems in this place.

Edit 2: Try to work not as

Code: Select all

const char *ptr = (const char*)str.mb_str();
but

Code: Select all

wxCharBuffer buf(1);
buf = str.mb_str();
const char *ptr = (const char *)buf;
Post Reply