[mapguide-internals] std::string not thread safe on Linux

[Trevor Wekel]

Hi Traian,

The only thing that worries me about "const wchar_t*" is the period of time between the return of the "Get" function call and the assignment to  std::wstring.  Although unlikely, the owning object could be deleted before the assignment to std::wstring occurs.

class A
{
public:
A(const wchar_t* str) {myString = str;}
const wchar_t* GetAString() { return myString.c_str(); }
private:
std::wstring myString;
}

(This code should be safe)
std::wstring str = objA->GetAString();
delete objA;
wprintf("%s\n",str.c_str());


(And this is not)
const wchar_t* pStr = objA->GetAString();
delete objA;
std::wstring str = pStr;
wprintf("%s\n",str.c_str());


In this case "delete objA;" is pretty obvious and most developers would catch it.  But if objA was a reference counted Ptr<> class with references held on multiple threads, the string destruct behaviour would be a little less obvious.  By returning a std::wstring, a copy of the string is automatically made and objA can be safely deleted at any point.

const wchar_t* pStr = NULL;
{
  Ptr<A> objA = someObject->GiveMeAnA(); 
  pStr = objA->GetAString();
}
wprintf("%s\n",pStr);  // is a crash if objA no longer exists.  It depends on who else holds a reference to objA.


Regards,
Trevor


-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Traian Stanev
Sent: August 5, 2010 6:08 PM
To: 'MapGuide Internals Mail List'
Subject: RE: [mapguide-internals] std::string not thread safe on Linux


Hi Trevor,

I specifically suggested returning const wchar_t* -- notice the const, in cases where it currently returns CREFSTRING. I did not suggest replacing all STL strings by manual string allocations. The const makes sure that the caller does not attempt to free the string. Doing this (and only this) will not have any lifetime issues, since that const string pointer is owned by its STRING parent, and it will be freed by STL like it is today.

Any code that calls such a function and assigns the result to a STRING today, will not have to change, while gaining speed and thread safety.

Traian


-----Original Message-----
From: mapguide-internals-bounces at lists.osgeo.org [mailto:mapguide-internals-bounces at lists.osgeo.org] On Behalf Of Trevor Wekel
Sent: Thursday, August 05, 2010 5:46 PM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] std::string not thread safe on Linux

Hi Traian,

The APIs in question are internal (C++ only) so I could return a const wchar_t*.  However, all of the published MapGuide APIs return STRING (std::wstring).  For the sake of consistency, I chose to return STRING.

We could make a wholesale change of STRING to wchar_t* but then we would have to manually deal with string lifetime, allocation and deallocation.  This would make MapGuide C++ development even more complicated.  I would not veto a change to raw wchar_t* but I would give it a "-0" vote because I believe a thread-safe C++ string class is a much better approach from a maintainability perspective.

I do not like string allocation either.  It is wasteful, useless, and sucks back performance.

On the other hand, design patterns in a multi-threaded open source code base are very important.  Consistent patterns that work (ie. are thread-safe) insulate developers from having to think about thread-safety for every line of code they write.  If we are truly serious about community involvement, I believe we should also consider ease of implementation for individuals that do not have 6+ of experience with the guts of MapGuide.

If we find a thread-safe, reference counted C++ class (ICU UnicodeString?) that avoids allocation I would be all for it.

Regards,
Trevor