From 09fdfe358ca161cd003ba5fb0390565cf2b7049d Mon Sep 17 00:00:00 2001 From: James Turner Date: Sat, 1 Sep 2018 18:53:36 +0100 Subject: [PATCH] Reset: ensure Nasal shutdown is clean Some improvements / bullet-proofing while trying to track down the slow-on-reset issue. --- src/Scripting/NasalSys.cxx | 85 ++++++++++++++++++++++-------- src/Scripting/NasalSys.hxx | 41 +++++++------- src/Scripting/NasalSys_private.hxx | 16 ++++++ 3 files changed, 101 insertions(+), 41 deletions(-) diff --git a/src/Scripting/NasalSys.cxx b/src/Scripting/NasalSys.cxx index c97fbacf5..d8534e483 100644 --- a/src/Scripting/NasalSys.cxx +++ b/src/Scripting/NasalSys.cxx @@ -90,7 +90,6 @@ void FGNasalModuleListener::valueChanged(SGPropertyNode*) ////////////////////////////////////////////////////////////////////////// - class TimerObj : public SGReferenced { public: @@ -103,15 +102,17 @@ public: char nm[128]; snprintf(nm, 128, "nasal-timer-%p", this); _name = nm; - _gcRoot = sys->gcSave(f); - _gcSelf = sys->gcSave(self); + _gcRoot = naGCSave(f); + _gcSelf = naGCSave(self); + sys->addPersistentTimer(this); } virtual ~TimerObj() { stop(); - _sys->gcRelease(_gcRoot); - _sys->gcRelease(_gcSelf); + naGCRelease(_gcRoot); + naGCRelease(_gcSelf); + _sys->removePersistentTimer(this); } bool isRunning() const { return _isRunning; } @@ -168,7 +169,7 @@ public: // event manager). _isRunning = false; - naRef *args = NULL; + naRef *args = nullptr; _sys->callMethod(_func, _self, 0, args, naNil() /* locals */); } @@ -966,22 +967,24 @@ void FGNasalSys::shutdown() shutdownNasalPositioned(); - map::iterator it, end = _listener.end(); - for(it = _listener.begin(); it != end; ++it) - delete it->second; + for (auto l : _listener) + delete l.second; _listener.clear(); - NasalCommandDict::iterator j = _commands.begin(); - for (; j != _commands.end(); ++j) { - globals->get_commands()->removeCommand(j->first); + for (auto c : _commands) { + globals->get_commands()->removeCommand(c.first); } _commands.clear(); - std::vector::iterator k = _moduleListeners.begin(); - for(; k!= _moduleListeners.end(); ++k) - delete *k; + for(auto ml : _moduleListeners) + delete ml; _moduleListeners.clear(); + for (auto t : _nasalTimers) { + delete t; + } + _nasalTimers.clear(); + naClearSaved(); _string = naNil(); // will be freed by _context @@ -994,6 +997,15 @@ void FGNasalSys::shutdown() _globals = naNil(); naGC(); + + // Destroy all queued ghosts : important to ensure persistent timers are + // destroyed now. + nasal::ghostProcessDestroyList(); + + if (!_persistentTimers.empty()) { + SG_LOG(SG_NASAL, SG_DEV_WARN, "Extant persistent timer count:" << _persistentTimers.size()); + } + _inited = false; } @@ -1374,11 +1386,8 @@ void FGNasalSys::setTimer(naContext c, int argc, naRef* args) name.append(std::to_string(naGetLine(c, 0))); // Generate and register a C++ timer handler - NasalTimer* t = new NasalTimer; - t->handler = handler; - t->gcKey = gcSave(handler); - t->nasal = this; - + NasalTimer* t = new NasalTimer(handler, this); + _nasalTimers.push_back(t); globals->get_event_mgr()->addEvent(name, t, &NasalTimer::timerExpired, delta.num, simtime); @@ -1387,7 +1396,10 @@ void FGNasalSys::setTimer(naContext c, int argc, naRef* args) void FGNasalSys::handleTimer(NasalTimer* t) { call(t->handler, 0, 0, naNil()); - gcRelease(t->gcKey); + auto it = std::find(_nasalTimers.begin(), _nasalTimers.end(), t); + assert(it != _nasalTimers.end()); + _nasalTimers.erase(it); + delete t; } int FGNasalSys::gcSave(naRef r) @@ -1402,12 +1414,27 @@ void FGNasalSys::gcRelease(int key) //------------------------------------------------------------------------------ -void FGNasalSys::NasalTimer::timerExpired() + +NasalTimer::NasalTimer(naRef h, FGNasalSys* sys) : + handler(h), nasal(sys) +{ + assert(sys); + gcKey = naGCSave(handler); +} + +NasalTimer::~NasalTimer() +{ + naGCRelease(gcKey); +} + +void NasalTimer::timerExpired() { nasal->handleTimer(this); - delete this; + // note handleTimer calls delete on us, don't do anything + // which requires 'this' to be valid here } + int FGNasalSys::_listenerId = 0; // setlistener(, [, [, ]]) @@ -1506,6 +1533,18 @@ void FGNasalSys::removeCommand(const std::string& name) _commands.erase(it); } +void FGNasalSys::addPersistentTimer(TimerObj* pto) +{ + _persistentTimers.push_back(pto); +} + +void FGNasalSys::removePersistentTimer(TimerObj* obj) +{ + auto it = std::find(_persistentTimers.begin(), _persistentTimers.end(), obj); + assert(it != _persistentTimers.end()); + _persistentTimers.erase(it); +} + ////////////////////////////////////////////////////////////////////////// // FGNasalListener class. diff --git a/src/Scripting/NasalSys.hxx b/src/Scripting/NasalSys.hxx index 502fd9dbc..f2cbf0fae 100644 --- a/src/Scripting/NasalSys.hxx +++ b/src/Scripting/NasalSys.hxx @@ -25,6 +25,8 @@ class SGCondition; class FGNasalModelData; class NasalCommand; class FGNasalModuleListener; +struct NasalTimer; ///< timer created by settimer +class TimerObj; ///< persistent timer created by maketimer namespace simgear { class BufferedLogCallback; } @@ -35,10 +37,9 @@ class FGNasalSys : public SGSubsystem public: FGNasalSys(); virtual ~FGNasalSys(); - virtual void init(); - virtual void shutdown(); - - virtual void update(double dt); + void init() override; + void shutdown() override; + void update(double dt) override; // Loads a nasal script from an external file and inserts it as a // global module of the specified name. @@ -167,19 +168,6 @@ private: // callback). bool _delay_load; - // - // FGTimer subclass for handling Nasal timer callbacks. - // See the implementation of the settimer() extension function for - // more notes. - // - struct NasalTimer { - virtual void timerExpired(); - virtual ~NasalTimer() {} - naRef handler; - int gcKey; - FGNasalSys* nasal; - }; - // Listener std::map _listener; std::vector _dead_listener; @@ -210,8 +198,25 @@ private: NasalCommandDict _commands; naRef _wrappedNodeFunc; -public: + + // track NasalTimer instances (created via settimer() call) - + // this allows us to clean these up on shutdown + std::vector _nasalTimers; + + // NasalTimer is a friend to invoke handleTimer and do the actual + // dispatch of the settimer-d callback + friend NasalTimer; + void handleTimer(NasalTimer* t); + + // track persistent timers. These are owned from the Nasal side, so we + // only track a non-owning reference here. + std::vector _persistentTimers; + + friend TimerObj; + + void addPersistentTimer(TimerObj* pto); + void removePersistentTimer(TimerObj* obj); }; #if 0 diff --git a/src/Scripting/NasalSys_private.hxx b/src/Scripting/NasalSys_private.hxx index 4eca7776a..017cfd01c 100644 --- a/src/Scripting/NasalSys_private.hxx +++ b/src/Scripting/NasalSys_private.hxx @@ -68,4 +68,20 @@ private: naRef _start_element, _end_element, _data, _pi; }; +// +// See the implementation of the settimer() extension function for +// more notes. +// +struct NasalTimer +{ + NasalTimer(naRef handler, FGNasalSys* sys); + + void timerExpired(); + ~NasalTimer(); + + naRef handler; + int gcKey = 0; + FGNasalSys* nasal = nullptr; +}; + #endif // of __NASALSYS_PRIVATE_HXX