21080: Fixes deadlock issue and almost all string resource leaks (#203)

howsoai · Jul 28, 2024 · a1f8a34 · a1f8a34
1 parent a89223c
commit a1f8a34
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 14 deletions.
diff --git a/src/Amalgam/ThreadPool.h b/src/Amalgam/ThreadPool.h
@@ -312,7 +312,11 @@ class ThreadPool
 		{
 			size_t prev_tasks_completed = numTasksCompleted.fetch_add(1);
 			if(prev_tasks_completed + 1 == numTasks)
+			{
+				//in theory, this lock may not be necessary, but in practice it is to prevent deadlock
+				std::unique_lock<std::mutex> lock(mutex);
 				cond_var.notify_all();
+			}
 		}
 
 	protected:

diff --git a/src/Amalgam/evaluablenode/EvaluableNodeManagement.h b/src/Amalgam/evaluablenode/EvaluableNodeManagement.h
@@ -139,14 +139,19 @@ class EvaluableNodeReference
 		unique = _unique;
 	}
 
-	//if check_reference is true, then it will additionally check any nodes referenced
-	__forceinline bool IsImmediateValue(bool check_reference = true)
+	//returns true if it is an immediate value stored in this EvaluableNodeReference
+	__forceinline bool IsImmediateValue()
+	{
+		return (value.nodeType != ENIVT_CODE);
+	}
+
+	//returns true if the type of whatever is stored is an immediate type
+	__forceinline bool IsImmediateValueType()
 	{
 		if(value.nodeType != ENIVT_CODE)
 			return true;
 
-		return (check_reference && 
-			(value.nodeValue.code == nullptr || value.nodeValue.code->IsImmediate()));
+		return (value.nodeValue.code == nullptr || value.nodeValue.code->IsImmediate());
 	}
 
 	__forceinline bool IsNonNullNodeReference()
@@ -630,15 +635,20 @@ class EvaluableNodeManager
 			return;
 
 		n->Invalidate();
-
 		ReclaimFreedNodesAtEnd();
 	}
 
 	//attempts to free the node reference
 	__forceinline void FreeNodeIfPossible(EvaluableNodeReference &enr)
 	{
-		if(enr.unique && !enr.IsImmediateValue(false) && !enr->GetNeedCycleCheck())
-			FreeNode(enr);
+		if(enr.IsImmediateValue())
+			enr.FreeImmediateResources();
+
+		if(enr.unique && enr != nullptr && !enr->GetNeedCycleCheck())
+		{
+			enr->Invalidate();
+			ReclaimFreedNodesAtEnd();
+		}
 	}
 
 	//frees all nodes
@@ -675,7 +685,7 @@ class EvaluableNodeManager
 	//attempts to free the node reference
 	__forceinline void FreeNodeTreeIfPossible(EvaluableNodeReference &enr)
 	{
-		if(enr.IsImmediateValue(false))
+		if(enr.IsImmediateValue())
 			enr.FreeImmediateResources();
 		else if(enr.unique)
 			FreeNodeTree(enr);

diff --git a/src/Amalgam/interpreter/Interpreter.cpp b/src/Amalgam/interpreter/Interpreter.cpp
@@ -566,7 +566,14 @@ StringInternPool::StringID Interpreter::InterpretNodeIntoStringIDValueWithRefere
 
 	if(result.IsImmediateValue())
 	{
-		return result.GetValue().GetValueAsStringIDWithReference();
+		auto &result_value = result.GetValue();
+
+		//reuse the reference if it has one
+		if(result_value.nodeType == ENIVT_STRING_ID)
+			return result_value.nodeValue.stringID;
+
+		//create new reference
+		return result_value.GetValueAsStringIDWithReference();
 	}
 	else //not immediate
 	{

diff --git a/src/Amalgam/interpreter/Interpreter.h b/src/Amalgam/interpreter/Interpreter.h
@@ -733,7 +733,7 @@ class Interpreter
 						*result = result_ref;
 
 						//only save the result if it's not immediate
-						if(!result_ref.IsImmediateValue(false))
+						if(!result_ref.IsImmediateValue())
 							resultsSaver.SetStackLocation(results_saver_location, *result);
 					}
 

diff --git a/src/Amalgam/interpreter/InterpreterOpcodesLogic.cpp b/src/Amalgam/interpreter/InterpreterOpcodesLogic.cpp
@@ -53,7 +53,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_AND(EvaluableNode *en, boo
 
 		cur = InterpretNode(cn, immediate_result);
 
-		if(cur.IsImmediateValue(false))
+		if(cur.IsImmediateValue())
 		{
 			if(!cur.GetValue().GetValueAsBoolean())
 				return AllocReturn(false, immediate_result);
@@ -158,7 +158,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_NOT(EvaluableNode *en, boo
 		return EvaluableNodeReference::Null();
 
 	auto cur = InterpretNodeForImmediateUse(ocn[0], immediate_result);
-	if(cur.IsImmediateValue(false))
+	if(cur.IsImmediateValue())
 	{
 		bool is_true = cur.GetValue().GetValueAsBoolean();
 		return AllocReturn(!is_true, immediate_result);

diff --git a/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp b/src/Amalgam/interpreter/InterpreterOpcodesTransformations.cpp
@@ -1209,7 +1209,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_REMOVE(EvaluableNode *en,
 	EvaluableNodeReference removed_node = EvaluableNodeReference(nullptr, container.unique && !container->GetNeedCycleCheck());
 
 	//if not a list, then just remove individual element
-	if(indices.IsImmediateValue())
+	if(indices.IsImmediateValueType())
 	{
 		if(container->IsAssociativeArray())
 		{
@@ -1316,7 +1316,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_KEEP(EvaluableNode *en, bo
 	auto indices = InterpretNodeForImmediateUse(ocn[1], true);
 
 	//if immediate then just keep individual element
-	if(indices.IsImmediateValue())
+	if(indices.IsImmediateValueType())
 	{
 		if(container->IsAssociativeArray())
 		{