Bugfix: Local events for changed attributes aren't processed at transaction commit time.

Fixes #3605

components/replication/replicationcomponent.cpp

@@ -35,7 +35,7 @@ void ReplicationComponent::Start(void)
 	DynamicObject::OnTransactionClosing.connect(boost::bind(&ReplicationComponent::TransactionClosingHandler, this, _1));
 
 	Endpoint::OnConnected.connect(boost::bind(&ReplicationComponent::EndpointConnectedHandler, this, _1));
-	
+
 	m_Endpoint->RegisterTopicHandler("config::ObjectUpdate",
 	    boost::bind(&ReplicationComponent::RemoteObjectUpdateHandler, this, _3));
 	m_Endpoint->RegisterTopicHandler("config::ObjectRemoved",
@@ -155,7 +155,7 @@ void ReplicationComponent::LocalObjectUnregisteredHandler(const DynamicObject::P
 	    MakeObjectMessage(object, "config::ObjectRemoved", 0, false));
 }
 
-void ReplicationComponent::TransactionClosingHandler(const set<DynamicObject::Ptr>& modifiedObjects)
+void ReplicationComponent::TransactionClosingHandler(const set<DynamicObject *>& modifiedObjects)
 {
 	if (modifiedObjects.empty())
 		return;
@@ -164,7 +164,9 @@ void ReplicationComponent::TransactionClosingHandler(const set<DynamicObject::Pt
 	msgbuf << "Sending " << modifiedObjects.size() << " replication updates.";
 	Logger::Write(LogDebug, "replication", msgbuf.str());
 
-	BOOST_FOREACH(const DynamicObject::Ptr& object, modifiedObjects) {
+	BOOST_FOREACH(DynamicObject *robject, modifiedObjects) {
+		DynamicObject::Ptr object = robject->GetSelf();
+
 		if (!ShouldReplicateObject(object))
 				continue;
 
@@ -204,7 +206,7 @@ void ReplicationComponent::RemoteObjectUpdateHandler(const RequestMessage& reque
 		object = dtype->CreateObject(update);
 
 		if (source == EndpointManager::GetInstance()->GetIdentity()) {
-			/* the peer sent us an object that was originally created by us - 
+			/* the peer sent us an object that was originally created by us -
 			 * however it was deleted locally so we have to tell the peer to destroy
 			 * its copy of the object. */
 			EndpointManager::GetInstance()->SendMulticastMessage(m_Endpoint,

components/replication/replicationcomponent.h

@@ -41,7 +41,7 @@ private:
 
 	void LocalObjectRegisteredHandler(const DynamicObject::Ptr& object);
 	void LocalObjectUnregisteredHandler(const DynamicObject::Ptr& object);
-	void TransactionClosingHandler(const set<DynamicObject::Ptr>& modifiedObjects);
+	void TransactionClosingHandler(const set<DynamicObject *>& modifiedObjects);
 
 	void RemoteObjectUpdateHandler(const RequestMessage& request);
 	void RemoteObjectRemovedHandler(const RequestMessage& request);

lib/base/dynamicobject.cpp

@@ -22,11 +22,11 @@
 using namespace icinga;
 
 double DynamicObject::m_CurrentTx = 0;
-set<DynamicObject::Ptr> DynamicObject::m_ModifiedObjects;
+set<DynamicObject *> DynamicObject::m_ModifiedObjects;
 
 boost::signal<void (const DynamicObject::Ptr&)> DynamicObject::OnRegistered;
 boost::signal<void (const DynamicObject::Ptr&)> DynamicObject::OnUnregistered;
-boost::signal<void (const set<DynamicObject::Ptr>&)> DynamicObject::OnTransactionClosing;
+boost::signal<void (const set<DynamicObject *>&)> DynamicObject::OnTransactionClosing;
 
 DynamicObject::DynamicObject(const Dictionary::Ptr& serializedObject)
 	: m_ConfigTx(0)
@@ -44,7 +44,22 @@ DynamicObject::DynamicObject(const Dictionary::Ptr& serializedObject)
 	/* apply config state from the config item/remote update;
 	 * The DynamicObject::Create function takes care of restoring
 	 * non-config state after the object has been fully constructed */
-	InternalApplyUpdate(serializedObject, Attribute_Config, true);
+	ApplyUpdate(serializedObject, Attribute_Config);
+}
+
+DynamicObject::~DynamicObject(void)
+{
+	m_ModifiedObjects.erase(this);
+}
+
+void DynamicObject::SendLocalUpdateEvents(void)
+{
+	map<String, Value, string_iless>::iterator it;
+	for (it = m_ModifiedAttributes.begin(); it != m_ModifiedAttributes.end(); it++) {
+		OnAttributeChanged(it->first, it->second);
+	}
+
+	m_ModifiedAttributes.clear();
 }
 
 Dictionary::Ptr DynamicObject::BuildUpdate(double sinceTx, int attributeTypes) const
@@ -87,12 +102,6 @@ Dictionary::Ptr DynamicObject::BuildUpdate(double sinceTx, int attributeTypes) c
 
 void DynamicObject::ApplyUpdate(const Dictionary::Ptr& serializedUpdate,
     int allowedTypes)
-{
-	InternalApplyUpdate(serializedUpdate, allowedTypes, false);
-}
-
-void DynamicObject::InternalApplyUpdate(const Dictionary::Ptr& serializedUpdate,
-    int allowedTypes, bool suppressEvents)
 {
 	double configTx = 0;
 	if ((allowedTypes & Attribute_Config) != 0 &&
@@ -126,7 +135,7 @@ void DynamicObject::InternalApplyUpdate(const Dictionary::Ptr& serializedUpdate,
 		if (!HasAttribute(it->first))
 			RegisterAttribute(it->first, static_cast<DynamicAttributeType>(type));
 
-		InternalSetAttribute(it->first, data, tx, suppressEvents, true);
+		InternalSetAttribute(it->first, data, tx, true);
 	}
 }
 
@@ -160,7 +169,7 @@ Value DynamicObject::Get(const String& name) const
 }
 
 void DynamicObject::InternalSetAttribute(const String& name, const Value& data,
-    double tx, bool suppressEvent, bool allowEditConfig)
+    double tx, bool allowEditConfig)
 {
 	DynamicAttribute attr;
 	attr.Type = Attribute_Transient;
@@ -184,10 +193,12 @@ void DynamicObject::InternalSetAttribute(const String& name, const Value& data,
 	if (tt.first->second.Type & Attribute_Config)
 		m_ConfigTx = tx;
 
-	if (!suppressEvent) {
-		m_ModifiedObjects.insert(GetSelf());
-		OnAttributeChanged(name, oldValue);
-	}
+	m_ModifiedObjects.insert(this);
+
+	/* Use insert() rather than [] so we don't overwrite
+	 * an existing oldValue if the attribute was previously
+	 * changed in the same transaction */
+	m_ModifiedAttributes.insert(make_pair(name, oldValue));
 }
 
 Value DynamicObject::InternalGetAttribute(const String& name) const
@@ -454,6 +465,10 @@ void DynamicObject::BeginTx(void)
 
 void DynamicObject::FinishTx(void)
 {
+	BOOST_FOREACH(DynamicObject *object, m_ModifiedObjects) {
+		object->SendLocalUpdateEvents();
+	}
+
 	OnTransactionClosing(m_ModifiedObjects);
 	m_ModifiedObjects.clear();
 

lib/base/dynamicobject.h

@@ -79,6 +79,7 @@ public:
 	typedef AttributeMap::const_iterator AttributeConstIterator;
 
 	DynamicObject(const Dictionary::Ptr& serializedObject);
+	~DynamicObject(void);
 
 	Dictionary::Ptr BuildUpdate(double sinceTx, int attributeTypes) const;
 	void ApplyUpdate(const Dictionary::Ptr& serializedUpdate, int allowedTypes);
@@ -95,7 +96,7 @@ public:
 
 	static boost::signal<void (const DynamicObject::Ptr&)> OnRegistered;
 	static boost::signal<void (const DynamicObject::Ptr&)> OnUnregistered;
-	static boost::signal<void (const set<DynamicObject::Ptr>&)> OnTransactionClosing;
+	static boost::signal<void (const set<DynamicObject *>&)> OnTransactionClosing;
 
 	ScriptTask::Ptr InvokeMethod(const String& method,
 	    const vector<Value>& arguments, ScriptTask::CompletionCallback callback);
@@ -135,17 +136,19 @@ protected:
 	virtual void OnInitCompleted(void);
 
 private:
-	void InternalSetAttribute(const String& name, const Value& data, double tx, bool suppressEvent = false, bool allowEditConfig = false);
+	void InternalSetAttribute(const String& name, const Value& data, double tx, bool allowEditConfig = false);
 	Value InternalGetAttribute(const String& name) const;
+	void SendLocalUpdateEvents(void);
 
 	AttributeMap m_Attributes;
+	map<String, Value, string_iless> m_ModifiedAttributes;
 	double m_ConfigTx;
 
 	static double m_CurrentTx;
 
-	static set<DynamicObject::Ptr> m_ModifiedObjects;
-
-	void InternalApplyUpdate(const Dictionary::Ptr& serializedUpdate, int allowedTypes, bool suppressEvents);
+	/* This has to be a set of raw pointers because the DynamicObject
+	 * constructor has to be able to insert objects into this list. */
+	static set<DynamicObject *> m_ModifiedObjects;
 
 	friend class DynamicType; /* for OnInitCompleted */
 };

lib/icinga/host.cpp

@@ -45,7 +45,7 @@ void Host::OnInitCompleted(void)
 	HostGroup::InvalidateMembersCache();
 	DowntimeProcessor::InvalidateDowntimeCache();
 
-	Event::Post(boost::bind(&Host::UpdateSlaveServices, this));
+	UpdateSlaveServices();
 }
 
 Host::~Host(void)