Re: [Flightgear-devel] [Flightgear-commitlogs] [SimGear] branch next updated: props: Over-protect nodeOriginsLock

[Julian S. <ju...@op...>]

On Sun, 18 Jun 2023 09:54:14 +0100
James Hogan via Flightgear-devel
<fli...@li...> wrote:

> On Saturday, 17 June 2023 23:05:30 BST Julian Smith wrote:
> > Apologies if i've misunderstood, but this patch seems a little
> > questionable - it seems to be hiding a problem rather than trying to
> > gather information that could be used to fix it.  
> 
> No worries, I'm really not a big fan of it either.

BTW apologies for not noticing the thread about this already here on
flightgear-devel.

> 
> > Could you give some more details of the failure mode we are trying
> > to cope with here?  

> 
> It seems that on certain Mac platforms, the lock is no longer usable
> after it has been destroyed resulting in exceptions when used by
> destructing property nodes, presumably like the one Fahim Dalvi
> quoted yesterday:
> 
> libc++abi: terminating with uncaught exception of type
> std::__1::system_error: mutex lock failed: Invalid argument
> 
> So yeh its a destructor ordering thing, and it'd be better if the
> properties were cleaned up first. Given that its not a problem on
> Linux or I think Windows I figured its best to just make it work in
> that case.

Thanks for this.

I've managed to provoke the problem on Linux by creating a global
SGPropertyNode_ptr with `SGPropertyNode_ptr g_test1(new
SGPropertyNode);` in simgear/simgear/props/props.cxx itself, before
(i.e. above in the source code) the `NecroLock nodeOriginsLock;` line.

Globals are destroyed in the reverse order in which they were
constructed, so this guarantees that g_test1's destructor will be
called /after/ nodeOriginsLock's destructor, so the former's call to
SGSourceLocation() will try to use a destructed NecroLock.

This reproducer only works reliably because everything is happening in
the same compilation unit. Order of initialisation between compilation
units is undefined, so i suspect it's just luck that things are
currently working on Linux and Windows.

> 
> > [If the problem is that properties are being manipulated after the
> > static global `std::shared_mutex nodeOriginsLock` is being
> > destructed by the C++ runtime, and for some reason we cannot avoid
> > this happening, then maybe we could put nodeOriginsLock on the heap
> > and never delete it, e.g. with `static std::shared_mutex&
> > nodeOriginsLock = *new std::shared_mutex`?]  
> 
> Sure, that does seem better. I'll give that a try and report back
> here with a patch for a kind Mac user to test.

A possibly more elegant solution might be to make all SGPropertyNode
constructors call a new dummy function in NecroLock(), which will ensure
that the global nodeOriginsLock is created before all SGPropertyNode's.
So nodeOriginsLock will be correspondingly destroyed /after/ all
SGPropertyNode's.

Arguably sticking nodeOriginsLock on the heap is a simpler solution
though...?


- Jules

-- 
http://op59.net