OgreTerrainGroup thread safety regarding Terrain creation #587

SNiLD · 2017-11-27T08:54:35Z

TerrainGroup will use Terrain instance inside thread but allows destroying it while it's being processed in the thread.

…und thread.

SNiLD · 2017-11-27T11:08:17Z

This change also kind of makes LoadRequest::loadingTaskNum redundant but I don't want to remove it so that backwards compatibility does not break.

paroj · 2017-11-27T14:17:12Z

I am not very familiar with the Terrain component API. Is TerrainGroup supposed to be inherited from? If not you can remove LoadRequest::loadingTaskNum as it is protected.

paroj · 2017-11-27T14:19:34Z

Components/Terrain/src/OgreTerrainGroup.cpp

+        if (it != mTerrainLoadRequests.end())
+        {
+            // We would like to abort the request here but WorkQueue does not provide proper tools for this
+            // (the data we need will be destroyed if we call abortRequest()).


which data are you referring to here? As far as I can see abortRequest() will only delete ResourceRequest::loadParams

You are probably referring to the TerrainSlot*. Would calling abortRequest() and immediately deleting it here work?

You're correct that the problem is TerrainSlot*. It can't be deleted however since Terrain* would also be deleted and that is used in the thread. If WorkQueue provided a method to abort request if it was not being processed right now it would be fine. That way we would have tools to abort all other requests except the one that's currently being processed.

WorkQueue has function abortPendingRequestsByChannel(uint16 channel) so if there was function like abortPendingRequest(RequestID id) as well it could be done.

if there was function like abortPendingRequest(RequestID id) as well it could be done.

this will probably be useful for others as well. So I would prefer this solution over the currently proposed workaround.

I'll make this change but currently I'm a bit busy so it might take a few days.

SNiLD · 2017-11-28T07:52:11Z

I think @sinbad meant TerrainGroup to be inherited from. Many of the important functions like define*() are virtual.

paroj · 2017-11-28T09:59:02Z

I think @sinbad meant TerrainGroup to be inherited from. Many of the important functions like define*() are virtual.

I would not assume that this is intentional. At some point he just made everything virtual (Java style).
A quick look at the samples and Ogitor showed that neither are inheriting from TerrainGroup. I assume you do neither - so feel free to consider everything protected to be actually private.

* Added separate function to abort pending requests. * Renamed TerrainLoadRequests to TerrainPrepareRequests. * Removed obsolete LoadRequest::loadingTaskNum.

SNiLD · 2017-12-04T12:49:48Z

This should work now.

Disappointingly I found out that calling Terrain::prepare() from background thread is not safe anyways since another terrain might try to access these resources (height data and quadtree) when they're neighbours. Fix to make Terrain::prepare() thread safe should be a separate issue though.

SNiLD · 2017-12-04T13:03:16Z

Actually there could be at least semi-remedy to this if TerrainGroup had disconnectNeighbour() as well like connectNeighbour(). I'll try this as well.

SNiLD · 2017-12-04T13:21:44Z

Components/Terrain/src/OgreTerrainGroup.cpp

-                freeTerrainSlotInstance(lreq.slot);
-                return;
-            }
+            freeTerrainSlotInstance(lreq.slot);


There should be return; after the free here. I'll try if this fixes the neighbour issue.

Ok that did not fix the issue, unfortunately.

SNiLD · 2017-12-04T13:42:14Z

TerrainGroup::disconnectNeighbour() does not make sense since Terrain will remove itself from neighbours when it's destroyed. The only case where it would be needed is when terrain instance prepare is aborted and is currently being processed in TerrainGroup. However once that function returns it will check for mTerrainPrepareRequests and free the slot if it was not found so it is never even connected.

Making Terrain thread safe regarding the neighbour access should be separate issue as stated before.

paroj · 2017-12-04T15:18:48Z

👍 from my side. Is this ready to merge?

SNiLD · 2017-12-04T15:19:04Z

Yes this is ready.

paroj · 2017-12-05T10:46:12Z

after this merge the "Terrain" sample crashes. Can you reproduce?

t == NULL here:

ogre/Samples/Terrain/include/Terrain.h

Lines 778 to 786 in 12b4b40

    
                   mTerrainGroup->loadAllTerrains(true); 
        
           #endif 
        
                   if (mTerrainsImported) 
        
                   { 
        
                       TerrainGroup::TerrainIterator ti = mTerrainGroup->getTerrainIterator(); 
        
                       while(ti.hasMoreElements()) 
        
                       { 
        
                           Terrain* t = ti.getNext()->instance;

SNiLD · 2017-12-05T10:55:02Z

Yes, it crashes for me too. Instances taken from mTerrainGroup->getTerrainIterator() seem to be null. Maybe I've missed something. Let me investigate.

SNiLD · 2017-12-05T11:01:08Z

Components/Terrain/src/OgreTerrainGroup.cpp

+                Root::getSingleton().getWorkQueue()->addRequest(
+                    mWorkQueueChannel, WORKQUEUE_LOAD_REQUEST,
+                    Any(req), 0, synchronous);
+            mTerrainPrepareRequests.insert(TerrainPrepareRequestMap::value_type(slot, id));


This is the problem line. When synchronous is true it won't go into background thread and there won't be anything in mTerrainPrepareRequests so it will be deleted.

local fix:

TerrainPrepareRequestMap::iterator iter = mTerrainPrepareRequests.insert(TerrainPrepareRequestMap::value_type(slot, 0)).first; iter->second = Root::getSingleton().getWorkQueue()->addRequest( mWorkQueueChannel, WORKQUEUE_LOAD_REQUEST, Any(req), 0, synchronous);

You can't insert to that iterator at that point anymore since it will be invalid at that point. This is how I'd fix it:

std::pair<TerrainPrepareRequestMap::iterator, bool> ret = mTerrainPrepareRequests.insert(TerrainPrepareRequestMap::value_type(slot, 0)); assert(ret.second == true); WorkQueue::RequestID id = Root::getSingleton().getWorkQueue()->addRequest( mWorkQueueChannel, WORKQUEUE_LOAD_REQUEST, Any(req), 0, synchronous); if (!synchronous) ret.first->second = id;

Ok this fix stops Terrain sample from crashing at start.

ah.. yes.. I figured. seems to be properly fixed this way

paroj · 2017-12-05T11:02:53Z

inserting in mTerrainPrepareRequests after doing a synchronous request is too late. But even when fixing this, the sample crashes on shutdown. Please investigate.

SNiLD · 2017-12-05T11:23:35Z

What is the proper way to merge that change as well? I pushed it to the same branch that you've already merged.

paroj · 2017-12-05T11:25:46Z

you have to create a new pull-request using the same branch.

Do not remove terrain instance if it's being processed in the backgro…

2d8eaf7

…und thread.

paroj reviewed Nov 27, 2017

View reviewed changes

Better abort logic and cleanup

decdfff

* Added separate function to abort pending requests. * Renamed TerrainLoadRequests to TerrainPrepareRequests. * Removed obsolete LoadRequest::loadingTaskNum.

SNiLD commented Dec 4, 2017

View reviewed changes

Return after free.

70331d1

paroj merged commit f6b63e4 into OGRECave:master Dec 4, 2017

SNiLD deleted the TerrainGroup-thread-safety branch December 4, 2017 16:34

SNiLD commented Dec 5, 2017

View reviewed changes

SNiLD restored the TerrainGroup-thread-safety branch December 5, 2017 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OgreTerrainGroup thread safety regarding Terrain creation #587

OgreTerrainGroup thread safety regarding Terrain creation #587

SNiLD commented Nov 27, 2017 •

edited

Loading

SNiLD commented Nov 27, 2017

paroj commented Nov 27, 2017

paroj Nov 27, 2017

paroj Nov 27, 2017

SNiLD Nov 28, 2017

SNiLD Nov 28, 2017

paroj Nov 28, 2017

SNiLD Nov 28, 2017

SNiLD commented Nov 28, 2017

paroj commented Nov 28, 2017

SNiLD commented Dec 4, 2017

SNiLD commented Dec 4, 2017

SNiLD Dec 4, 2017

SNiLD Dec 4, 2017

SNiLD commented Dec 4, 2017

paroj commented Dec 4, 2017

SNiLD commented Dec 4, 2017

paroj commented Dec 5, 2017 •

edited

Loading

SNiLD commented Dec 5, 2017

SNiLD Dec 5, 2017

paroj Dec 5, 2017 •

edited

Loading

SNiLD Dec 5, 2017 •

edited

Loading

SNiLD Dec 5, 2017

paroj Dec 5, 2017

paroj commented Dec 5, 2017

SNiLD commented Dec 5, 2017

paroj commented Dec 5, 2017

OgreTerrainGroup thread safety regarding Terrain creation #587

OgreTerrainGroup thread safety regarding Terrain creation #587

Conversation

SNiLD commented Nov 27, 2017 • edited Loading

SNiLD commented Nov 27, 2017

paroj commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNiLD commented Nov 28, 2017

paroj commented Nov 28, 2017

SNiLD commented Dec 4, 2017

SNiLD commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNiLD commented Dec 4, 2017

paroj commented Dec 4, 2017

SNiLD commented Dec 4, 2017

paroj commented Dec 5, 2017 • edited Loading

SNiLD commented Dec 5, 2017

Choose a reason for hiding this comment

paroj Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

SNiLD Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paroj commented Dec 5, 2017

SNiLD commented Dec 5, 2017

paroj commented Dec 5, 2017

SNiLD commented Nov 27, 2017 •

edited

Loading

paroj commented Dec 5, 2017 •

edited

Loading

paroj Dec 5, 2017 •

edited

Loading

SNiLD Dec 5, 2017 •

edited

Loading