20883: Fixes bug where calling contained entities could crash (#170)

src/Amalgam/entity/Entity.cpp

@@ -427,18 +427,13 @@ EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, Execut
 	StringInternPool::StringID label_sid, EvaluableNode *call_stack, bool on_self, Interpreter *calling_interpreter,
 	std::vector<EntityWriteListener *> *write_listeners, PrintListener *print_listener
 #ifdef MULTITHREAD_SUPPORT
-	, Concurrency::ReadLock *entity_read_lock
+	, Concurrency::ReadLock *enm_lock
 #endif
 	)
 {
 	if(!on_self && IsLabelPrivate(label_sid))
 		return EvaluableNodeReference(nullptr, true);
 
-#ifdef MULTITHREAD_SUPPORT
-	if(calling_interpreter != nullptr)
-		calling_interpreter->memoryModificationLock.unlock();
-#endif
-
 	EvaluableNode *node_to_execute = nullptr;
 	if(label_sid <= StringInternPool::EMPTY_STRING_ID)	//if not specified, then use root
 		node_to_execute = evaluableNodeManager.GetRootNode();
@@ -452,35 +447,23 @@ EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, Execut
 
 	//if label not found or no code, can't do anything
 	if(node_to_execute == nullptr)
-	{
-	#ifdef MULTITHREAD_SUPPORT
-		//put lock back in place
-		if(calling_interpreter != nullptr)
-			calling_interpreter->memoryModificationLock.lock();
-	#endif
 		return EvaluableNodeReference::Null();
-	}
 
 	size_t a_priori_entity_storage = evaluableNodeManager.GetNumberOfUsedNodes();
 
 	Interpreter interpreter(&evaluableNodeManager, max_num_steps, max_num_nodes, randomStream.CreateOtherStreamViaRand(),
 		write_listeners, print_listener, this, calling_interpreter);
 
 #ifdef MULTITHREAD_SUPPORT
-	interpreter.memoryModificationLock = Concurrency::ReadLock(interpreter.evaluableNodeManager->memoryModificationMutex);
-	if(entity_read_lock != nullptr)
-		entity_read_lock->unlock();
+	if(enm_lock == nullptr)
+		interpreter.memoryModificationLock = Concurrency::ReadLock(evaluableNodeManager.memoryModificationMutex);
+	else
+		interpreter.memoryModificationLock = std::move(*enm_lock);
 #endif
 
 	EvaluableNodeReference retval = interpreter.ExecuteNode(node_to_execute, call_stack);
 	num_steps_executed = interpreter.GetNumStepsExecuted();
 
-#ifdef MULTITHREAD_SUPPORT
-	//make sure have lock before copy into destination_temp_enm
-	if(calling_interpreter != nullptr)
-		calling_interpreter->memoryModificationLock.lock();
-#endif
-
 	//find difference in entity size
 	size_t post_entity_storage = evaluableNodeManager.GetNumberOfUsedNodes() + interpreter.GetNumEntityNodesAllocated();
 	if(a_priori_entity_storage > post_entity_storage)
@@ -489,9 +472,10 @@ EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, Execut
 		num_nodes_allocated = post_entity_storage - a_priori_entity_storage;
 
 #ifdef MULTITHREAD_SUPPORT
-	interpreter.memoryModificationLock.unlock();
+	if(enm_lock != nullptr)
+		*enm_lock = std::move(interpreter.memoryModificationLock);
 #endif
-		
+
 	return retval;
 }
 

src/Amalgam/entity/Entity.h

@@ -187,13 +187,12 @@ class Entity
 	// Uses max_num_steps as the maximum number of operations that can be executed by this and any subordinate operations called. If max_num_steps is 0, then it will execute unlimeted steps
 	// Uses max_num_nodes as the maximum number of nodes that can be allocated in memory by this and any subordinate operations called. If max_num_nodes is 0, then it will allow unlimited allocations
 	// If on_self is true, then it will be allowed to access private variables
-	// If entity_read_lock is specified, then it will unlock prior to execution after the interpreter's memoryModificationLock is locked
-	// potentially writing anything out to destination_temp_enm
+	// If enm_lock is specified, it should be a lock on this entity's evaluableNodeManager.memoryModificationMutex
 	EvaluableNodeReference Execute(ExecutionCycleCount max_num_steps, ExecutionCycleCount &num_steps_executed, size_t max_num_nodes, size_t &num_nodes_allocated,
 		StringInternPool::StringID label_sid, EvaluableNode *call_stack, bool on_self = false, Interpreter *calling_interpreter = nullptr,
 		std::vector<EntityWriteListener *> *write_listeners = nullptr, PrintListener *print_listener = nullptr
 	#ifdef MULTITHREAD_SUPPORT
-		, Concurrency::ReadLock *entity_read_lock = nullptr
+		, Concurrency::ReadLock *enm_lock = nullptr
 	#endif
 		);
 
@@ -203,15 +202,15 @@ class Entity
 		std::string &label_name, EvaluableNode *call_stack, bool on_self = false, Interpreter *calling_interpreter = nullptr,
 		std::vector<EntityWriteListener *> *write_listeners = nullptr, PrintListener *print_listener = nullptr
 	#ifdef MULTITHREAD_SUPPORT
-		, Concurrency::ReadLock *entity_read_lock = nullptr
+		, Concurrency::ReadLock *enm_lock = nullptr
 	#endif
 		)
 	{
 		StringInternPool::StringID label_sid = string_intern_pool.GetIDFromString(label_name);
 		return Execute(max_num_steps, num_steps_executed, max_num_nodes, num_nodes_allocated,
 			label_sid, call_stack, on_self, calling_interpreter, write_listeners, print_listener
 		#ifdef MULTITHREAD_SUPPORT
-			, entity_read_lock
+			, enm_lock
 		#endif
 			);
 	}

src/Amalgam/entity/EntityExternalInterface.cpp

@@ -199,14 +199,22 @@ std::string EntityExternalInterface::ExecuteEntityJSON(std::string &handle, std:
 		return "";
 
 	EvaluableNodeManager &enm = bundle->entity->evaluableNodeManager;
+#ifdef MULTITHREAD_SUPPORT
+	//lock memory before allocating call stack
+	Concurrency::ReadLock enm_lock(enm.memoryModificationMutex);
+#endif
 	EvaluableNodeReference args = EvaluableNodeReference(EvaluableNodeJSONTranslation::JsonToEvaluableNode(&enm, json), true);
 
 	auto call_stack = Interpreter::ConvertArgsToCallStack(args, enm);
 
 	ExecutionCycleCount max_num_steps = 0, num_steps_executed = 0;
 	size_t max_num_nodes = 0, num_nodes_allocated = 0;
 	EvaluableNodeReference returned_value = bundle->entity->Execute(max_num_steps, num_steps_executed, max_num_nodes,
-		num_nodes_allocated, label, call_stack, false, nullptr, &bundle->writeListeners, bundle->printListener);
+		num_nodes_allocated, label, call_stack, false, nullptr, &bundle->writeListeners, bundle->printListener
+#ifdef MULTITHREAD_SUPPORT
+		, &enm_lock
+#endif
+	);
 
 	//ConvertArgsToCallStack always adds an outer list that is safe to free
 	enm.FreeNode(call_stack);
@@ -221,7 +229,7 @@ bool EntityExternalInterface::EntityListenerBundle::SetEntityValueAtLabel(std::s
 	StringInternPool::StringID label_sid = string_intern_pool.GetIDFromString(label_name);
 
 	EntityWriteReference entity_wr(entity);
-#ifdef MULTITHREAD_SUPPORT
+#ifdef MULTITHREAD_INTERFACE
 	//make a full copy of the entity in case any other threads are operating on it
 	entity->SetRoot(entity->GetRoot(), false);
 #endif

src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp

@@ -503,9 +503,16 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_ENTITY_and_CALL_ENTIT
 
 	//get a write lock on the entity
 	EntityReadReference called_entity = InterpretNodeIntoRelativeSourceEntityReadReference(ocn[0]);
+
 	if(called_entity == nullptr)
 		return EvaluableNodeReference::Null();
 
+#ifdef MULTITHREAD_SUPPORT
+	//lock memory before allocating call stack, then can release the entity lock
+	Concurrency::ReadLock enm_lock(called_entity->evaluableNodeManager.memoryModificationMutex);
+	called_entity.lock.unlock();
+#endif
+
 	EvaluableNodeReference call_stack;
 	if(called_entity == curEntity)
 	{
@@ -522,20 +529,30 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_ENTITY_and_CALL_ENTIT
 		call_stack = ConvertArgsToCallStack(called_entity_args, called_entity->evaluableNodeManager);
 	}
 
+#ifdef MULTITHREAD_SUPPORT
+	//this interpreter is no longer executing
+	memoryModificationLock.unlock();
+#endif
+
 	ExecutionCycleCount num_steps_executed = 0;
 	size_t num_nodes_allocated = 0;
 	EvaluableNodeReference result = called_entity->Execute(num_steps_allowed, num_steps_executed,
 		num_nodes_allowed, num_nodes_allocated,
 		entity_label_sid, call_stack, called_entity == curEntity, this, cur_write_listeners, printListener
 	#ifdef MULTITHREAD_SUPPORT
-		, &called_entity.lock
+		, &enm_lock
 	#endif
 		);
 
 	//accumulate costs of execution
 	curExecutionStep += num_steps_executed;
 	curNumExecutionNodesAllocatedToEntities += num_nodes_allocated;
 
+#ifdef MULTITHREAD_SUPPORT
+	//this interpreter is executing again
+	memoryModificationLock.lock();
+#endif
+
 	//call opcodes should consume the outer return opcode if there is one
 	if(result.IsNonNullNodeReference() && result->GetType() == ENT_RETURN)
 		result = RemoveTopConcludeOrReturnNode(result, &called_entity->evaluableNodeManager);
@@ -635,16 +652,21 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_CONTAINER(EvaluableNo
 			num_nodes_allowed = std::min(num_nodes_allowed, GetRemainingNumExecutionNodes());
 	}
 
-	//lock the current entity
+	//obtain a lock on the container
 	EntityReadReference cur_entity(curEntity);
 	StringInternPool::StringID cur_entity_sid = curEntity->GetIdStringId();
 	EntityReadReference container(curEntity->GetContainer());
 	if(container == nullptr)
 		return EvaluableNodeReference::Null();
-
 	//don't need the curEntity as a reference anymore -- can free the lock
 	cur_entity = EntityReadReference();
 
+#ifdef MULTITHREAD_SUPPORT
+	//lock memory before allocating call stack, then can release the entity lock
+	Concurrency::ReadLock enm_lock(container->evaluableNodeManager.memoryModificationMutex);
+	container.lock.unlock();
+#endif
+
 	//copy arguments to container, free args from this entity
 	EvaluableNodeReference called_entity_args = container->evaluableNodeManager.DeepAllocCopy(args);
 	evaluableNodeManager->FreeNodeTreeIfPossible(args);
@@ -655,19 +677,29 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_CONTAINER(EvaluableNo
 	EvaluableNode *call_stack_args = call_stack->GetOrderedChildNodesReference()[0];
 	call_stack_args->SetMappedChildNode(ENBISI_accessing_entity, container->evaluableNodeManager.AllocNode(ENT_STRING, cur_entity_sid));
 
+#ifdef MULTITHREAD_SUPPORT
+	//this interpreter is no longer executing
+	memoryModificationLock.unlock();
+#endif
+
 	ExecutionCycleCount num_steps_executed = 0;
 	size_t num_nodes_allocated = 0;
 	EvaluableNodeReference result = container->Execute(num_steps_allowed, num_steps_executed, num_nodes_allowed, num_nodes_allocated,
 		container_label_sid, call_stack, false, this, writeListeners, printListener
 	#ifdef MULTITHREAD_SUPPORT
-		, &container.lock
+		, &enm_lock
 	#endif
 		);
 
 	//accumulate costs of execution
 	curExecutionStep += num_steps_executed;
 	curNumExecutionNodesAllocatedToEntities += num_nodes_allocated;
 
+#ifdef MULTITHREAD_SUPPORT
+	//this interpreter is executing again
+	memoryModificationLock.lock();
+#endif
+
 	//call opcodes should consume the outer return opcode if there is one
 	if(result.IsNonNullNodeReference() && result->GetType() == ENT_RETURN)
 		result = RemoveTopConcludeOrReturnNode(result, &container->evaluableNodeManager);

src/Amalgam/interpreter/InterpreterOpcodesEntityControl.cpp

@@ -653,10 +653,20 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_LOAD_ENTITY_and_LOAD_PERSI
 
 	EntityExternalInterface::LoadEntityStatus status;
 	std::string random_seed = destination_entity_parent->CreateRandomStreamFromStringAndRand(resource_name);
+
+#ifdef MULTITHREAD_SUPPORT
+	//this interpreter is no longer executing
+	memoryModificationLock.unlock();
+#endif
 
 	Entity *loaded_entity = asset_manager.LoadEntityFromResourcePath(resource_name, file_type,
 		persistent, true, escape_filename, escape_contained_filenames, random_seed, this, status);
 
+#ifdef MULTITHREAD_SUPPORT
+	//this interpreter is executing again
+	memoryModificationLock.lock();
+#endif
+
 	//handle errors
 	if(!status.loaded)
 		return EvaluableNodeReference::Null();