diff --git a/simgear/props/props.cxx b/simgear/props/props.cxx index cc3a1b19..b7bf5367 100644 --- a/simgear/props/props.cxx +++ b/simgear/props/props.cxx @@ -48,6 +48,18 @@ using std::stringstream; using namespace simgear; + +struct SGPropertyNodeListeners +{ + /* This keeps a count of the current number of nested invocations of + forEachListener(). If non-zero, other code higher up the stack is iterating + _items[] so for example code must not erase items in the vector. */ + int _num_iterators = 0; + + std::vector _items; +}; + + //////////////////////////////////////////////////////////////////////// // Local classes. //////////////////////////////////////////////////////////////////////// @@ -968,7 +980,7 @@ SGPropertyNode::~SGPropertyNode () if (_listeners) { vector::iterator it; - for (it = _listeners->begin(); it != _listeners->end(); ++it) + for (it = _listeners->_items.begin(); it != _listeners->_items.end(); ++it) (*it)->unregister_property(this); delete _listeners; } @@ -2392,8 +2404,21 @@ SGPropertyNode::addChangeListener (SGPropertyChangeListener * listener, bool initial) { if (_listeners == 0) - _listeners = new vector; - _listeners->push_back(listener); + _listeners = new SGPropertyNodeListeners; + + /* If there's a nullptr entry (a listener that was unregistered), we + overwrite it. This ensures that listeners that routinely unregister+register + themselves don't make _listeners->_items grow unnecessarily. Otherwise simply + append. */ + auto it = std::find(_listeners->_items.begin(), _listeners->_items.end(), + (SGPropertyChangeListener*) nullptr + ); + if (it == _listeners->_items.end()) { + _listeners->_items.push_back(listener); + } + else { + *it = listener; + } listener->register_property(this); if (initial) listener->valueChanged(this); @@ -2405,14 +2430,29 @@ SGPropertyNode::removeChangeListener (SGPropertyChangeListener * listener) if (_listeners == 0) return; vector::iterator it = - find(_listeners->begin(), _listeners->end(), listener); - if (it != _listeners->end()) { - _listeners->erase(it); - listener->unregister_property(this); - if (_listeners->empty()) { - vector* tmp = _listeners; - _listeners = 0; - delete tmp; + find(_listeners->_items.begin(), _listeners->_items.end(), listener); + if (it != _listeners->_items.end()) { + if (_listeners->_num_iterators) { + /* _listeners._items is currently being iterated further up the stack in + this thread by one or more nested invocations of forEachListener(), so + we must be careful to not break these iterators. So we must not delete + _listeners. And we must not erase this entry from the vector because that + could cause the next listener to be missed out. + + So we simply set this entry to nullptr (nullptr items are skipped by + forEachListener()). When all invocations of forEachListener() have + finished iterating and it is safe to modify things again, it will clean + up any nullptr entries in _listeners->_items. */ + *it = nullptr; + listener->unregister_property(this); + } + else { + _listeners->_items.erase(it); + listener->unregister_property(this); + if (_listeners->_items.empty()) { + delete _listeners; + _listeners = 0; + } } } } @@ -2461,15 +2501,67 @@ SGPropertyNode::fireChildrenRemovedRecursive() } } +/* Calls for each item in _listeners. We are careful to skip nullptr +entries in _listeners->items[], which can be created if listeners are removed +while we are iterating. */ +static void forEachListener( + SGPropertyNode* node, + SGPropertyNodeListeners*& _listeners, + std::function callback + ) +{ + if (!_listeners) return; + + _listeners->_num_iterators += 1; + + /* We need to use an index here when iterating _listeners->_items, not an + iterator. This is because a listener may add new listeners, causing the + vector to be reallocated, which would invalidate any iterator. */ + for (size_t i = 0; i < _listeners->_items.size(); ++i) { + auto listener = _listeners->_items[i]; + if (listener) { + try { + callback(listener); + } + catch (std::exception& e) { + SG_LOG(SG_GENERAL, SG_ALERT, "Ignoring exception from property callback: " << e.what()); + } + } + } + + _listeners->_num_iterators -= 1; + + if (_listeners->_num_iterators == 0) { + /* Remove any items that have been set to nullptr. */ + _listeners->_items.erase( + std::remove(_listeners->_items.begin(), _listeners->_items.end(), (SGPropertyChangeListener*) nullptr), + _listeners->_items.end() + ); + if (_listeners->_items.empty()) { + delete _listeners; + _listeners = nullptr; + } + } +} + +int SGPropertyNode::nListeners() const +{ + if (!_listeners) return 0; + int n = 0; + for (auto listener: _listeners->_items) { + if (listener) n += 1; + } + return n; +} + void SGPropertyNode::fireValueChanged (SGPropertyNode * node) { - if (_listeners != 0) { - for (unsigned int i = 0; i < _listeners->size(); i++) { - if ((*_listeners)[i]) - (*_listeners)[i]->valueChanged(node); - } - } + forEachListener( + node, + _listeners, + [&](SGPropertyChangeListener* listener) { listener->valueChanged(node);} + ); if (_parent != 0) _parent->fireValueChanged(node); } @@ -2478,11 +2570,11 @@ void SGPropertyNode::fireChildAdded (SGPropertyNode * parent, SGPropertyNode * child) { - if (_listeners != 0) { - for (unsigned int i = 0; i < _listeners->size(); i++) { - (*_listeners)[i]->childAdded(parent, child); - } - } + forEachListener( + parent, + _listeners, + [&](SGPropertyChangeListener* listener) { listener->childAdded(parent, child);} + ); if (_parent != 0) _parent->fireChildAdded(parent, child); } @@ -2491,11 +2583,11 @@ void SGPropertyNode::fireChildRemoved (SGPropertyNode * parent, SGPropertyNode * child) { - if (_listeners != 0) { - for (unsigned int i = 0; i < _listeners->size(); i++) { - (*_listeners)[i]->childRemoved(parent, child); - } - } + forEachListener( + parent, + _listeners, + [&](SGPropertyChangeListener* listener) { listener->childRemoved(parent, child);} + ); if (_parent != 0) _parent->fireChildRemoved(parent, child); } diff --git a/simgear/props/props.hxx b/simgear/props/props.hxx index d1e8dd6a..828fe7c5 100644 --- a/simgear/props/props.hxx +++ b/simgear/props/props.hxx @@ -759,6 +759,9 @@ private: }; +struct SGPropertyNodeListeners; + + /** * A node in a property tree. */ @@ -1713,7 +1716,7 @@ public: /** * Get the number of listeners. */ - int nListeners () const { return _listeners ? (int)_listeners->size() : 0; } + int nListeners () const; /** @@ -1847,7 +1850,7 @@ private: char * string_val; } _local_val; - std::vector * _listeners; + SGPropertyNodeListeners* _listeners; // Pass name as a pair of iterators template diff --git a/simgear/props/props_test.cxx b/simgear/props/props_test.cxx index 14211040..51fe0977 100644 --- a/simgear/props/props_test.cxx +++ b/simgear/props/props_test.cxx @@ -451,13 +451,17 @@ void defineSamplePropertyTree(SGPropertyNode_ptr root) class TestListener : public SGPropertyChangeListener { public: - TestListener(SGPropertyNode* root, bool recursive = false) : - _root(root) {} + TestListener(SGPropertyNode* root, bool recursive = false, bool self_unregister = false) : + _root(root), _self_unregister(self_unregister) {} virtual void valueChanged(SGPropertyNode* node) override { std::cout << "saw value changed for:" << node->getPath() << std::endl; valueChangeCount[node]++; + if (_self_unregister) { + std::cout << "valueChanged(): calling removeChangeListener() to self-remove\n"; + node->removeChangeListener(this); + } } int checkValueChangeCount(const std::string& path) const @@ -533,6 +537,7 @@ public: } private: SGPropertyNode* _root; + bool _self_unregister; std::map valueChangeCount; std::vector adds; @@ -942,6 +947,13 @@ void testDeleterListener() SG_VERIFY(ensureNListeners(tree, 0)); } + // Self-unregister. prior to 2019-07-08 this would segv. + { + std::shared_ptr l1( new TestListener(tree.get(), true /* recursive */, true /*self_unregister*/)); + tree->setFloatValue("position/body/sub/self-unregister", 0.1); + tree->getNode("position/body/sub/self-unregister")->addChangeListener(l1.get()); + tree->setFloatValue("position/body/sub/self-unregister", 0.2); + } } int main (int ac, char ** av)