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

Fri Aug 6 01:01:31 EDT 2010

Hi Trevor,

Are you finding that code bellow will crash on Linux ?
I used your example but changed from const wchar_t* to wstring so ref
count is used.

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

(Not safe)
std::wstring Str = objA->GetAString();
delete objA;
wprintf("%s\n",Str.c_str());

Haris

On Fri, Aug 6, 2010 at 5:22 AM, Trevor Wekel
<trevor_wekel at otxsystems.com> wrote:
> That would work for internal C++ API's only.  Published APIs cannot use parameter references due to the implementation differences in PHP/.Net/Java.  For the sake of consistency, I would prefer to return STRING for the internal C++ APIs instead of using function(REFSTRING ret).  From what I can tell, the APIs in question are not used in any tight loops so the performance implications should be minimal.
>
> So far, the main processing loops (rendering, stylization, etc) have not been impacted by this but my Grinder test is not hitting any rendering code either.  I will keep my fingers crossed.
>
> I still need to apply my full set of patches to a Windows build.  Once I have done that, I will attach the patch files to a defect so we can all look at them.  I am also out of town for the next few days.  If I get a chance to build on Windows tonight, I will post the patches tonight.
>
> And one other note - the one hour load test on Linux was successful.  Hooray!  Now I have to figure out how to get The Grinder to stop logging that gigabyte data file for each hour of runtime.  My "client" VM doesn't have enough free disk space to deal with a 24 hour run...
>
>
> 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 9:05 PM
> To: 'MapGuide Internals Mail List'
> Subject: RE: [mapguide-internals] std::string not thread safe on Linux
>
>
> I don't think it's both because with an object lifetime bug, it would be crashing on Windows as well and it would happen even with a single threaded app.
>
> Anyway, overall I'd recommend this approach for performance and thread safety:
>
>
> void function (REFSTRING ret)
> {
>    ret = my_String.c_str(); //c_str() breaks refcount sharing
> }
>
> It does one allocation and also does not give a possibility to remember a stale reference.
>
> 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 11:01 PM
> To: MapGuide Internals Mail List
> Subject: RE: [mapguide-internals] std::string not thread safe on Linux
>
> 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
>
> _______________________________________________
> 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
>
>