From 92ad6a61fa0065e723bb458d0d0d47a5d9d76dc7 Mon Sep 17 00:00:00 2001
From: wraitii <wraitii@wildfiregames.com>
Date: Mon, 23 Sep 2019 06:38:16 +0000
Subject: [PATCH] Fix OOS introduced by pathfinder threading preparation diff
 d592bf9cb6

Following d592bf9cb6, paths were computed at the end of turn N, and then
messages were sent at the beginning of turn N+1. However, the path
requests were removed at the end of turn N and so weren't serialised,
and neither were computed paths. This meant rejoiners would OOS when the
game was serialised with pending path results.

To fix this in preparation for threading, the architecture needs to
change slightly so that requests are kept and serialised correctly, and
rejoiners can compute the paths after deserialisation, in order to send
the messages at the beginning of turn N+1.

Fixes #5604

Reported By: elexis
Differential Revision: https://code.wildfiregames.com/D2317
This was SVN commit r22979.
---
 .../simulation2/components/CCmpPathfinder.cpp | 34 ++++++++++++-------
 .../components/CCmpPathfinder_Common.h        |  3 +-
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/source/simulation2/components/CCmpPathfinder.cpp b/source/simulation2/components/CCmpPathfinder.cpp
index 65e0f1e2f1..c0811e8a6b 100644
--- a/source/simulation2/components/CCmpPathfinder.cpp
+++ b/source/simulation2/components/CCmpPathfinder.cpp
@@ -185,6 +185,15 @@ void CCmpPathfinder::HandleMessage(const CMessage& msg, bool UNUSED(global))
 		m_TerrainDirty = true;
 		UpdateGrid();
 		break;
+	case MT_Deserialized:
+		UpdateGrid();
+		// In case we were serialised with requests pending, we need to process them.
+		if (!m_ShortPathRequests.empty() || !m_LongPathRequests.empty())
+		{
+			ENSURE(CmpPtr<ICmpObstructionManager>(GetSystemEntity()));
+			StartProcessingMoves(false);
+		}
+		break;
 	}
 }
 
@@ -773,6 +782,10 @@ void CCmpPathfinder::FetchAsyncResultsAndSendMessages()
 {
 	PROFILE2("FetchAsyncResults");
 
+	// We may now clear existing requests.
+	m_ShortPathRequests.clear();
+	m_LongPathRequests.clear();
+
 	// WARNING: the order in which moves are pulled must be consistent when using 1 or n workers.
 	// We fetch in the same order we inserted in, but we push moves backwards, so this works.
 	std::vector<PathResult> results;
@@ -794,8 +807,8 @@ void CCmpPathfinder::FetchAsyncResultsAndSendMessages()
 
 void CCmpPathfinder::StartProcessingMoves(bool useMax)
 {
-	std::vector<LongPathRequest> longRequests = PopMovesToProcess(m_LongPathRequests, useMax, m_MaxSameTurnMoves);
-	std::vector<ShortPathRequest> shortRequests = PopMovesToProcess(m_ShortPathRequests, useMax, m_MaxSameTurnMoves - longRequests.size());
+	std::vector<LongPathRequest> longRequests = GetMovesToProcess(m_LongPathRequests, useMax, m_MaxSameTurnMoves);
+	std::vector<ShortPathRequest> shortRequests = GetMovesToProcess(m_ShortPathRequests, useMax, m_MaxSameTurnMoves - longRequests.size());
 
 	PushRequestsToWorkers(longRequests);
 	PushRequestsToWorkers(shortRequests);
@@ -805,25 +818,20 @@ void CCmpPathfinder::StartProcessingMoves(bool useMax)
 }
 
 template <typename T>
-std::vector<T> CCmpPathfinder::PopMovesToProcess(std::vector<T>& requests, bool useMax, size_t maxMoves)
+std::vector<T> CCmpPathfinder::GetMovesToProcess(std::vector<T>& requests, bool useMax, size_t maxMoves)
 {
-	std::vector<T> poppedRequests;
+	// Keep the original requests in which we need to serialize.
+	std::vector<T> copiedRequests;
 	if (useMax)
 	{
 		size_t amount = std::min(requests.size(), maxMoves);
 		if (amount > 0)
-		{
-			poppedRequests.insert(poppedRequests.begin(), std::make_move_iterator(requests.end() - amount), std::make_move_iterator(requests.end()));
-			requests.erase(requests.end() - amount, requests.end());
-		}
+			copiedRequests.insert(copiedRequests.begin(), requests.end() - amount, requests.end());
 	}
 	else
-	{
-		poppedRequests.swap(requests);
-		requests.clear();
-	}
+		copiedRequests = requests;
 
-	return poppedRequests;
+	return copiedRequests;
 }
 
 template <typename T>
diff --git a/source/simulation2/components/CCmpPathfinder_Common.h b/source/simulation2/components/CCmpPathfinder_Common.h
index 418dc9937f..f3333ae939 100644
--- a/source/simulation2/components/CCmpPathfinder_Common.h
+++ b/source/simulation2/components/CCmpPathfinder_Common.h
@@ -87,6 +87,7 @@ protected:
 public:
 	static void ClassInit(CComponentManager& componentManager)
 	{
+		componentManager.SubscribeToMessageType(MT_Deserialized);
 		componentManager.SubscribeToMessageType(MT_Update);
 		componentManager.SubscribeToMessageType(MT_RenderSubmit); // for debug overlays
 		componentManager.SubscribeToMessageType(MT_TerrainChanged);
@@ -225,7 +226,7 @@ public:
 	virtual void StartProcessingMoves(bool useMax);
 
 	template <typename T>
-	std::vector<T> PopMovesToProcess(std::vector<T>& requests, bool useMax = false, size_t maxMoves = 0);
+	std::vector<T> GetMovesToProcess(std::vector<T>& requests, bool useMax = false, size_t maxMoves = 0);
 
 	template <typename T>
 	void PushRequestsToWorkers(std::vector<T>& from);