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

[Trevor Wekel]

It may be a bit of both.  The internally refcounted data structure for std::string on Linux is more thread-unsafe because the strings are pointing at the same data.  On Windows, std:string is always deep copied making it impossible to get the "double free".

Returning the CREFSTRING on Linux makes refcount sharing possible.  Returning a STRING also makes it possible.  Returning a STRING created using myString.c_str() breaks the sharing cycle.  And passing "const wchar_t*" also makes sharing possible.

Yuck. 

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 8:47 PM
To: 'MapGuide Internals Mail List'
Subject: RE: [mapguide-internals] std::string not thread safe on Linux


OK it was my impression that the problem you fixed was the sharing of resources under assignment of STRING to another STRING, not that the owning object was being deleted before the reference is used.

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 10:44 PM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] std::string not thread safe on Linux

Hi Traian,

It can happen with CREFSTRING.  We do not crash on startup because the string has to be used by the other thread after the original thread has deleted the owning object.  It's a race condition which depends on thread scheduling. If the other thread gets it's time slice before the owning thread then it is still referencing a valid string and everything is ok.

That's why I changed CREFSTRING to STRING.

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 8:38 PM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] std::string not thread safe on Linux


Hi Trevor,

Isn't this the exact same behavior as code which currently returns CREFSTRING? If the owning object dies, the reference becomes bogus -- same as the const wchar_t* case. So if the existing code is safe against that, then it is not a problem -- presumably it is already assigning the string to a STRING and not keeping the const reference around.

The problem you describe can hypothetically happen, but not in the current MapGuide codebase, because if it did, we would be crashing basically in the first millisecond of starting the server process.

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 10:28 PM
To: MapGuide Internals Mail List
Subject: RE: [mapguide-internals] std::string not thread safe on Linux

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

_______________________________________________
mapguide-internals mailing list
mapguide-internals at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals

_______________________________________________
mapguide-internals mailing list
mapguide-internals at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/mapguide-internals