From 246feef85fedd548f2c198831d7d6e7e3be53e12 Mon Sep 17 00:00:00 2001 From: ThorstenB Date: Sun, 8 Jan 2012 13:28:49 +0100 Subject: [PATCH] #553: MP-model-loading related segfault MP aircraft are loaded by a separate OSG thread (introduced after FG2.4.0). The OSG thread also calls the "modelLoaded" callback. However, we mustn't allow the OSG thread to call FGNasalModelData::modelLoaded directly: FGNasalModelData isn't thread-safe. There are obvious issues (_callCount++;), tricky issues like calling the Nasal _parser_ or creating hashes and modifying the global Nasal namespace. It doesn't use locks to protect against another thread executing a Nasal context or running garbage collection. It also executes Nasal code itself (the model's "load" hook), which we also cannot allow in a separate thread... This patch returns all Nasal parts of MP-aircraft loading (parsing, module creation, execution) to the main thread, while keeping the multi-threaded OSG part (loading of MP-aircraft model files itself). The same issue exists with scenery models (see other commit). To summarize with 2 words: It s*cks... ;-) --- src/AIModel/AIBase.cxx | 65 +++++++++++++++++++++++++++--------------- src/AIModel/AIBase.hxx | 21 ++++++++++---- 2 files changed, 57 insertions(+), 29 deletions(-) diff --git a/src/AIModel/AIBase.cxx b/src/AIModel/AIBase.cxx index 07fc6c31a..a9d64e341 100644 --- a/src/AIModel/AIBase.cxx +++ b/src/AIModel/AIBase.cxx @@ -75,8 +75,7 @@ FGAIBase::FGAIBase(object_type ot, bool enableHot) : _refID( _newAIModelID() ), _otype(ot), _initialized(false), - _aimodel(0), - _fxpath(""), + _modeldata(0), _fx(0) { @@ -232,21 +231,28 @@ void FGAIBase::update(double dt) { pitch*speed ); _fx->set_velocity( velocity ); } - else if ((_aimodel)&&(fgGetBool("/sim/sound/aimodels/enabled",false))) + else if ((_modeldata)&&(_modeldata->needInitilization())) { - string fxpath = _aimodel->get_sound_path(); - if (fxpath != "") - { - _fxpath = fxpath; - props->setStringValue("sim/sound/path", _fxpath.c_str()); + // process deferred nasal initialization, + // which must be done in main thread + _modeldata->init(); - // initialize the sound configuration - SGSoundMgr *smgr = globals->get_soundmgr(); - stringstream name; - name << "aifx:"; - name << _refID; - _fx = new FGFX(smgr, name.str(), props); - _fx->init(); + // sound initialization + if (fgGetBool("/sim/sound/aimodels/enabled",false)) + { + string fxpath = _modeldata->get_sound_path(); + if (fxpath != "") + { + props->setStringValue("sim/sound/path", fxpath.c_str()); + + // initialize the sound configuration + SGSoundMgr *smgr = globals->get_soundmgr(); + stringstream name; + name << "aifx:"; + name << _refID; + _fx = new FGFX(smgr, name.str(), props); + _fx->init(); + } } } } @@ -324,8 +330,8 @@ bool FGAIBase::init(bool search_in_AI_path) else _installed = true; - _aimodel = new FGAIModelData(props); - osg::Node * mdl = SGModelLib::loadDeferredModel(f, props, _aimodel); + _modeldata = new FGAIModelData(props); + osg::Node * mdl = SGModelLib::loadDeferredModel(f, props, _modeldata); _model = new osg::LOD; _model->setName("AI-model range animation node"); @@ -912,7 +918,8 @@ int FGAIBase::_newAIModelID() { FGAIModelData::FGAIModelData(SGPropertyNode *root) : _nasal( new FGNasalModelData(root) ), - _path("") + _ready(false), + _initialized(false) { } @@ -923,9 +930,21 @@ FGAIModelData::~FGAIModelData() void FGAIModelData::modelLoaded(const string& path, SGPropertyNode *prop, osg::Node *n) { - const char* fxpath = prop->getStringValue("sound/path"); - if (fxpath) { - _path = string(fxpath); - } - _nasal->modelLoaded(path, prop, n); + // WARNING: All this is called in a separate OSG thread! Only use thread-safe stuff + // here that is fine to be run concurrently with stuff in the main loop! + if (_ready) + return; + _fxpath = _prop->getStringValue("sound/path"); + _prop = prop; + _path = path; + _ready = true; +} + +// do Nasal initialization (must be called in the main loop) +void FGAIModelData::init() +{ + // call FGNasalSys to create context and run hooks (not-thread safe!) + _nasal->modelLoaded(_path, _prop, 0); + _prop = 0; + _initialized = true; } diff --git a/src/AIModel/AIBase.hxx b/src/AIModel/AIBase.hxx index b4815c41a..b4782eb43 100644 --- a/src/AIModel/AIBase.hxx +++ b/src/AIModel/AIBase.hxx @@ -21,7 +21,6 @@ #define _FG_AIBASE_HXX #include -#include #include #include @@ -38,7 +37,6 @@ using std::string; -using std::list; class SGMaterial; class FGAIManager; @@ -230,9 +228,8 @@ private: bool _initialized; osg::ref_ptr _model; //The 3D model LOD object - osg::ref_ptr _aimodel; + osg::ref_ptr _modeldata; - string _fxpath; SGSharedPtr _fx; public: @@ -453,12 +450,24 @@ class FGAIModelData : public simgear::SGModelData { public: FGAIModelData(SGPropertyNode *root = 0); ~FGAIModelData(); + + /** osg callback, thread-safe */ void modelLoaded(const string& path, SGPropertyNode *prop, osg::Node *n); - inline string& get_sound_path() { return _path; }; + + /** init hook to be called after model is loaded. + * Not thread-safe. Call from main thread only. */ + void init(void); + + bool needInitilization(void) { return _ready && !_initialized;} + bool isInitialized(void) { return _initialized;} + inline std::string& get_sound_path() { return _fxpath;} private: FGNasalModelData *_nasal; - string _path; + SGPropertyNode_ptr _prop; + std::string _path, _fxpath; + bool _ready; + bool _initialized; }; #endif // _FG_AIBASE_HXX