[osg-users] Deleting still referenced object

[Robert Osfield]

Thanks for chipping in Laurens.  I have just had a look at simgear and
the DefaultCachePolicy::find(const string& fileName, const Options*
opt) that calls registry->getFromObjectCache(fileName) is definitely
problematic.  the getFromObjectCache() method only passes back a C
pointer, not a ref_ptr<> and has been proven to cause threading
issues.  The ObjectCache::getRefFromObjectCache() is the replacement
for this unsafe method and should always be used in threaded
applications.

The DefaultCachePolicy::find(..) method is also problematic as it
passes back a C pointer as well, so it should be changed to pass back
a ref_ptr<> so that every step of the way the subgraph that is being
passed around remains with a positive ref count.

The recent PR for the OSG just shifts the timing so make the crash
less common, but the bugs are still there.  From 3.4 onwards the OSG
has the tools to prevent these problems, but with the old unsafe API's
still around for compatibility unfortunately users aren't forced to do
the right thing.

I did a quick checkout of simgear and had a bash at fixing this issue
but am getting compile errors on unrelated code. The code I would
change though would be:

// original and thread unsafe ModelRegistry.cxx :
osg::Node* DefaultCachePolicy::find(const string& fileName,
                                    const Options* opt)
{
    Registry* registry = Registry::instance();
    osg::Node* cached
        = dynamic_cast<Node*>(registry->getFromObjectCache(fileName));
    if (cached)
        SG_LOG(SG_IO, SG_BULK, "Got cached model \""
               << fileName << "\"");
    else
        SG_LOG(SG_IO, SG_BULK, "Reading model \""
               << fileName << "\"");
    return cached;
}

Suggested improvement:
osg::ref_ptr<osg::Node> DefaultCachePolicy::find(const string& fileName,
                                    const Options* opt)
{
#if OSG_VERSION_LESS_THAN(3,4,0)
    osg::ref_ptr<osg::Object> cachedObject =
registry->getFromObjectCache(fileName);
#else
    osg::ref_ptr<osg::Object> cachedObject =
registry->getRefFromObjectCache(fileName);
#endif

    ref_ptr<osg::Node> cachedNode =
dynamic_cast<osg::Node*>(cachedObject.get());
    if (cached.valid())
        SG_LOG(SG_IO, SG_BULK, "Got cached model \""
               << fileName << "\"");
    else
        SG_LOG(SG_IO, SG_BULK, "Reading model \""
               << fileName << "\"");
    return cached;
}

Not the change to use ref_ptr<> where ownership is explicitly handled
and the use of ObjectCache::getRefFromObjectCache().

I will add a comment to the ObjectCache about getFromObjectCache()
being deprecated and not thread safe and give the recommendation of
changing to getRefFromObjectCache().

I would also recommend changing instances of
readNodeFile/readImageFile/readObjectFile() to readRefNodeFile() etc.
In the case of the SGReaderWRiterXML.cxx I'd change the line:

        modelResult = osgDB::readNodeFile(modelpath.local8BitStr(),
options.get());

To:

        modelResult =
osgDB::Registry::instance()->readNode(modelpath.local8BitStr(),
options.get());

As the ReadResult already has ref_ptr<> usage internally and is thread
safe already.

All these Ref version for file loading and cache came in existence to
address threading issues.  In hindsight the OSG should never have
provided the non ref_ptr<> versions.

Cheers,
Robert.

On Mon, 14 Jan 2019 at 10:55, Voerman, L. <l.voerman at rug.nl> wrote:
>
> Hi Richard,
> sorry to drop into the discussion so late, I think the problem is that you should call
> getRefFromObjectCache
> instead of
> getFromObjectCache
>
> available in osg versions above 3.3.4, this should close the gap where the refCount can touch zero.
> From your stack trace the call seems to come from the flightgear code:
>
> ot21-OpenThreads.dll!OpenThreads::Mutex::lock() Line 115 C++
> osg160-osgDB.dll!osgDB::ObjectCache::getFromObjectCache(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & fileName, const osgDB::Options * options) Line 99 C++
> fgfs.exe!simgear::DefaultCachePolicy::find(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & fileName, const osgDB::Options * opt) Line 724 C++
>
> Regards, Laurens.
>
> On Mon, Jan 14, 2019 at 8:28 AM Richard Harrison <rjh at zaretto.com> wrote:
>>
>> On 11/01/2019 07:38, Robert Osfield wrote:
>> > If you are able to characterise what is going on then let me know and
>> > I may be able to help spot a solution.  Having a small example that
>> >
>> For some reason my last message doesn't seem to have made it to this
>> list; it's on the forum[1]
>>
>> I've diagnosed what I think is the problem, I've got a solution and I've
>> tested it; what I'm really after is confirmation that I haven't missed
>> something.
>>
>> This relates to pull request
>> https://github.com/openscenegraph/OpenSceneGraph/pull/690
>>
>> -------------------
>>
>> [1]
>> http://forum.openscenegraph.org/viewtopic.php?p=75443&sid=f9ec59f5127e4760f6694c56b925f54a#75443
>>
>> _______________________________________________
>> osg-users mailing list
>> osg-users at lists.openscenegraph.org
>> http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org
>
> _______________________________________________
> osg-users mailing list
> osg-users at lists.openscenegraph.org
> http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org