Merge pull request #2522 from cuberite/FixLoaderGenRaceCondition

Fixed a race condition between chunk loader and generator.

src/Bindings/ManualBindings_World.cpp

@@ -348,11 +348,11 @@ static int tolua_cWorld_PrepareChunk(lua_State * tolua_S)
 		}
 
 		// cChunkCoordCallback override:
-		virtual void Call(int a_CBChunkX, int a_CBChunkZ) override
+		virtual void Call(int a_CBChunkX, int a_CBChunkZ, bool a_IsSuccess) override
 		{
 			if (m_Callback.IsValid())
 			{
-				m_LuaState.Call(m_Callback, a_CBChunkX, a_CBChunkZ);
+				m_LuaState.Call(m_Callback, a_CBChunkX, a_CBChunkZ, a_IsSuccess);
 			}
 
 			// This is the last reference of this object, we must delete it so that it doesn't leak:

src/ChunkDef.h

@@ -464,7 +464,8 @@ public:
 
 	virtual ~cChunkCoordCallback() {}
 
-	virtual void Call(int a_ChunkX, int a_ChunkZ) = 0;
+	/** Called with the chunk's coords, and an optional operation status flag for operations that support it. */
+	virtual void Call(int a_ChunkX, int a_ChunkZ, bool a_IsSuccess) = 0;
 } ;
 
 

src/ChunkMap.cpp

@@ -2396,10 +2396,10 @@ void cChunkMap::PrepareChunk(int a_ChunkX, int a_ChunkZ, std::unique_ptr<cChunkC
 		return;
 	}
 
-	// The chunk is present and lit, just call the callback:
+	// The chunk is present and lit, just call the callback, report as success:
 	if (a_Callback != nullptr)
 	{
-		a_Callback->Call(a_ChunkX, a_ChunkZ);
+		a_Callback->Call(a_ChunkX, a_ChunkZ, true);
 	}
 }
 
@@ -2432,9 +2432,19 @@ bool cChunkMap::GenerateChunk(int a_ChunkX, int a_ChunkZ, cChunkCoordCallback *
 			}
 
 			// cChunkCoordCallback override:
-			virtual void Call(int a_CBChunkX, int a_CBChunkZ) override
+			virtual void Call(int a_CBChunkX, int a_CBChunkZ, bool a_CBIsSuccess) override
 			{
-				// The chunk has been loaded or an error occurred, check if it's valid now:
+				// If success is reported, the chunk is already valid, no need to do anything else:
+				if (a_CBIsSuccess)
+				{
+					if (m_Callback != nullptr)
+					{
+						m_Callback->Call(a_CBChunkX, a_CBChunkZ, true);
+					}
+					return;
+				}
+
+				// The chunk failed to load, generate it:
 				cCSLock Lock(m_ChunkMap.m_CSLayers);
 				cChunkPtr CBChunk = m_ChunkMap.GetChunkNoLoad(a_CBChunkX, a_CBChunkZ);
 
@@ -2443,23 +2453,13 @@ bool cChunkMap::GenerateChunk(int a_ChunkX, int a_ChunkZ, cChunkCoordCallback *
 					// An error occurred, but we promised to call the callback, so call it even when there's no real chunk data:
 					if (m_Callback != nullptr)
 					{
-						m_Callback->Call(a_CBChunkX, a_CBChunkZ);
+						m_Callback->Call(a_CBChunkX, a_CBChunkZ, false);
 					}
 					return;
 				}
 
-				// If the chunk is not valid, queue it in the generator:
+				CBChunk->SetPresence(cChunk::cpQueued);
-				if (!CBChunk->IsValid())
+				m_World.GetGenerator().QueueGenerateChunk(a_CBChunkX, a_CBChunkZ, false, m_Callback);
-				{
-					m_World.GetGenerator().QueueGenerateChunk(a_CBChunkX, a_CBChunkZ, false, m_Callback);
-					return;
-				}
-
-				// The chunk was loaded, call the callback:
-				if (m_Callback != nullptr)
-				{
-					m_Callback->Call(a_CBChunkX, a_CBChunkZ);
-				}
 			}
 
 		protected:
@@ -2474,7 +2474,7 @@ bool cChunkMap::GenerateChunk(int a_ChunkX, int a_ChunkZ, cChunkCoordCallback *
 	// The chunk is valid, just call the callback:
 	if (a_Callback != nullptr)
 	{
-		a_Callback->Call(a_ChunkX, a_ChunkZ);
+		a_Callback->Call(a_ChunkX, a_ChunkZ, true);
 	}
 	return true;
 }

src/ChunkSender.cpp

@@ -27,7 +27,7 @@
 class cNotifyChunkSender :
 	public cChunkCoordCallback
 {
-	virtual void Call(int a_ChunkX, int a_ChunkZ) override
+	virtual void Call(int a_ChunkX, int a_ChunkZ, bool a_IsSuccess) override
 	{
 		cChunkSender & ChunkSender = m_ChunkSender;
 		m_World.DoWithChunk(

src/Generating/ChunkGenerator.cpp

@@ -248,13 +248,13 @@ void cChunkGenerator::Execute(void)
 			LastReportTick = clock();
 		}
 
-		// Skip the chunk if it's already generated and regeneration is not forced:
+		// Skip the chunk if it's already generated and regeneration is not forced. Report as success:
 		if (!item.m_ForceGenerate && m_ChunkSink->IsChunkValid(item.m_ChunkX, item.m_ChunkZ))
 		{
 			LOGD("Chunk [%d, %d] already generated, skipping generation", item.m_ChunkX, item.m_ChunkZ);
 			if (item.m_Callback != nullptr)
 			{
-				item.m_Callback->Call(item.m_ChunkX, item.m_ChunkZ);
+				item.m_Callback->Call(item.m_ChunkX, item.m_ChunkZ, true);
 			}
 			continue;
 		}
@@ -265,7 +265,7 @@ void cChunkGenerator::Execute(void)
 			LOGWARNING("Chunk generator overloaded, skipping chunk [%d, %d]", item.m_ChunkX, item.m_ChunkZ);
 			if (item.m_Callback != nullptr)
 			{
-				item.m_Callback->Call(item.m_ChunkX, item.m_ChunkZ);
+				item.m_Callback->Call(item.m_ChunkX, item.m_ChunkZ, false);
 			}
 			continue;
 		}
@@ -275,7 +275,7 @@ void cChunkGenerator::Execute(void)
 		DoGenerate(item.m_ChunkX, item.m_ChunkZ);
 		if (item.m_Callback != nullptr)
 		{
-			item.m_Callback->Call(item.m_ChunkX, item.m_ChunkZ);
+			item.m_Callback->Call(item.m_ChunkX, item.m_ChunkZ, true);
 		}
 		NumChunksGenerated++;
 	}  // while (!bStop)
@@ -296,8 +296,8 @@ void cChunkGenerator::DoGenerate(int a_ChunkX, int a_ChunkZ)
 	m_PluginInterface->CallHookChunkGenerated(ChunkDesc);
 
 	#ifdef _DEBUG
-	// Verify that the generator has produced valid data:
+		// Verify that the generator has produced valid data:
-	ChunkDesc.VerifyHeightmap();
+		ChunkDesc.VerifyHeightmap();
 	#endif
 
 	m_ChunkSink->OnChunkGenerated(ChunkDesc);

src/LightingThread.cpp

@@ -238,12 +238,12 @@ void cLightingThread::Execute(void)
 
 void cLightingThread::LightChunk(cLightingChunkStay & a_Item)
 {
-	// If the chunk is already lit, skip it:
+	// If the chunk is already lit, skip it (report as success):
 	if (m_World->IsChunkLighted(a_Item.m_ChunkX, a_Item.m_ChunkZ))
 	{
 		if (a_Item.m_CallbackAfter != nullptr)
 		{
-			a_Item.m_CallbackAfter->Call(a_Item.m_ChunkX, a_Item.m_ChunkZ);
+			a_Item.m_CallbackAfter->Call(a_Item.m_ChunkX, a_Item.m_ChunkZ, true);
 		}
 		return;
 	}
@@ -324,7 +324,7 @@ void cLightingThread::LightChunk(cLightingChunkStay & a_Item)
 
 	if (a_Item.m_CallbackAfter != nullptr)
 	{
-		a_Item.m_CallbackAfter->Call(a_Item.m_ChunkX, a_Item.m_ChunkZ);
+		a_Item.m_CallbackAfter->Call(a_Item.m_ChunkX, a_Item.m_ChunkZ, true);
 	}
 }
 

src/LightingThread.h

@@ -60,7 +60,8 @@ public:
 	
 	void Stop(void);
 	
-	/** Queues the entire chunk for lighting */
+	/** Queues the entire chunk for lighting.
+	The callback, if specified, is called after the lighting has been processed. */
 	void QueueChunk(int a_ChunkX, int a_ChunkZ, std::unique_ptr<cChunkCoordCallback> a_CallbackAfter);
 	
 	/** Blocks until the queue is empty or the thread is terminated */

src/SpawnPrepare.cpp

@@ -20,7 +20,7 @@ protected:
 
 	cSpawnPrepare & m_SpawnPrepare;
 	
-	virtual void Call(int a_ChunkX, int a_ChunkZ) override
+	virtual void Call(int a_ChunkX, int a_ChunkZ, bool a_IsSuccess) override
 	{
 		m_SpawnPrepare.PreparedChunkCallback(a_ChunkX, a_ChunkZ);
 	}

src/WorldStorage/WorldStorage.cpp

@@ -164,32 +164,6 @@ void cWorldStorage::QueueSaveChunk(int a_ChunkX, int a_ChunkZ, cChunkCoordCallba
 
 
 
-void cWorldStorage::UnqueueLoad(int a_ChunkX, int a_ChunkZ)
-{
-	m_LoadQueue.RemoveIf([=](cChunkCoordsWithCallback & a_Item)
-		{
-			return (a_Item.m_ChunkX == a_ChunkX) && (a_Item.m_ChunkZ == a_ChunkZ);
-		}
-	);
-}
-
-
-
-
-
-void cWorldStorage::UnqueueSave(const cChunkCoords & a_Chunk)
-{
-	m_SaveQueue.RemoveIf([=](cChunkCoordsWithCallback & a_Item)
-		{
-			return (a_Item.m_ChunkX == a_Chunk.m_ChunkX) && (a_Item.m_ChunkZ == a_Chunk.m_ChunkZ);
-		}
-	);
-}
-
-
-
-
-
 void cWorldStorage::InitSchemas(int a_StorageCompressionFactor)
 {
 	// The first schema added is considered the default
@@ -266,7 +240,7 @@ bool cWorldStorage::LoadOneChunk(void)
 	// Call the callback, if specified:
 	if (ToLoad.m_Callback != nullptr)
 	{
-		ToLoad.m_Callback->Call(ToLoad.m_ChunkX, ToLoad.m_ChunkZ);
+		ToLoad.m_Callback->Call(ToLoad.m_ChunkX, ToLoad.m_ChunkZ, res);
 	}
 	return res;
 }
@@ -286,19 +260,21 @@ bool cWorldStorage::SaveOneChunk(void)
 	}
 	
 	// Save the chunk, if it's valid:
+	bool Status = false;
 	if (m_World->IsChunkValid(ToSave.m_ChunkX, ToSave.m_ChunkZ))
 	{
 		m_World->MarkChunkSaving(ToSave.m_ChunkX, ToSave.m_ChunkZ);
 		if (m_SaveSchema->SaveChunk(cChunkCoords(ToSave.m_ChunkX, ToSave.m_ChunkZ)))
 		{
 			m_World->MarkChunkSaved(ToSave.m_ChunkX, ToSave.m_ChunkZ);
+			Status = true;
 		}
 	}
 
 	// Call the callback, if specified:
 	if (ToSave.m_Callback != nullptr)
 	{
-		ToSave.m_Callback->Call(ToSave.m_ChunkX, ToSave.m_ChunkZ);
+		ToSave.m_Callback->Call(ToSave.m_ChunkX, ToSave.m_ChunkZ, Status);
 	}
 	return true;
 }

src/WorldStorage/WorldStorage.h

@@ -63,13 +63,15 @@ public:
 
 	cWorldStorage(void);
 	~cWorldStorage();
-	
+
+	/** Queues a chunk to be loaded, asynchronously.
+	The callback, if specified, will be called with the result of the load operation. */
 	void QueueLoadChunk(int a_ChunkX, int a_ChunkZ, cChunkCoordCallback * a_Callback = nullptr);
+
+	/** Queues a chunk to be saved, asynchronously.
+	The callback, if specified, will be called with the result of the save operation. */
 	void QueueSaveChunk(int a_ChunkX, int a_ChunkZ, cChunkCoordCallback * a_Callback = nullptr);
 	
-	void UnqueueLoad(int a_ChunkX, int a_ChunkZ);
-	void UnqueueSave(const cChunkCoords & a_Chunk);
-	
 	bool Start(cWorld * a_World, const AString & a_StorageSchemaName, int a_StorageCompressionFactor);  // Hide the cIsThread's Start() method, we need to provide args
 	void Stop(void);  // Hide the cIsThread's Stop() method, we need to signal the event
 	void WaitForFinish(void);