Merge #5327

5327: Add CLockScope r=heinrich5991 a=def-

Remove unused lock and scope in threading.h

Not sure if this is preferred

We could also try switching to std::mutex and lock_guard

<!-- What is the motivation for the changes of this pull request -->

## Checklist

- [x] Tested the change ingame
- [ ] Provided screenshots if it is a visual change
- [ ] Tested in combination with possibly related configuration options
- [ ] Written a unit test if it works standalone, system.c especially
- [ ] Considered possible null pointers and out of bounds array indexing
- [ ] Changed no physics that affect existing maps
- [ ] Tested the change with [ASan+UBSan or valgrind's memcheck](https://github.com/ddnet/ddnet/#using-addresssanitizer--undefinedbehavioursanitizer-or-valgrinds-memcheck) (optional)


Co-authored-by: Dennis Felsing <dennis@felsin9.de>

CMakeLists.txt

@@ -1683,6 +1683,7 @@ set_src(BASE GLOB_RECURSE src/base
   hash_ctxt.h
   hash_libtomcrypt.cpp
   hash_openssl.cpp
+  lock_scope.h
   log.cpp
   log.h
   logger.h

src/base/lock_scope.h

@@ -0,0 +1,24 @@
+#ifndef BASE_LOCK_SCOPE_H
+#define BASE_LOCK_SCOPE_H
+
+#include "system.h"
+
+class SCOPED_CAPABILITY CLockScope
+{
+public:
+	CLockScope(LOCK Lock) ACQUIRE(Lock, m_Lock) REQUIRES(!Lock, !m_Lock) :
+		m_Lock(Lock)
+	{
+		lock_wait(m_Lock);
+	}
+
+	~CLockScope() RELEASE() REQUIRES(m_Lock)
+	{
+		lock_unlock(m_Lock);
+	}
+
+private:
+	LOCK m_Lock;
+};
+
+#endif

src/base/system.cpp

@@ -13,6 +13,7 @@
 
 #include "system.h"
 
+#include "lock_scope.h"
 #include "logger.h"
 
 #include <sys/stat.h>
@@ -624,11 +625,8 @@ void aio_write_newline(ASYNCIO *aio)
 
 int aio_error(ASYNCIO *aio)
 {
-	int result;
-	lock_wait(aio->lock);
-	result = aio->error;
-	lock_unlock(aio->lock);
-	return result;
+	CLockScope ls(aio->lock);
+	return aio->error;
 }
 
 void aio_free(ASYNCIO *aio)
@@ -644,23 +642,25 @@ void aio_free(ASYNCIO *aio)
 
 void aio_close(ASYNCIO *aio)
 {
-	lock_wait(aio->lock);
-	aio->finish = ASYNCIO_CLOSE;
-	lock_unlock(aio->lock);
+	{
+		CLockScope ls(aio->lock);
+		aio->finish = ASYNCIO_CLOSE;
+	}
 	sphore_signal(&aio->sphore);
 }
 
 void aio_wait(ASYNCIO *aio)
 {
 	void *thread;
-	lock_wait(aio->lock);
-	thread = aio->thread;
-	aio->thread = 0;
-	if(aio->finish == ASYNCIO_RUNNING)
 	{
-		aio->finish = ASYNCIO_EXIT;
+		CLockScope ls(aio->lock);
+		thread = aio->thread;
+		aio->thread = 0;
+		if(aio->finish == ASYNCIO_RUNNING)
+		{
+			aio->finish = ASYNCIO_EXIT;
+		}
 	}
-	lock_unlock(aio->lock);
 	sphore_signal(&aio->sphore);
 	thread_wait(thread);
 }

src/base/tl/threading.h

@@ -28,44 +28,4 @@ public:
 	}
 };
 
-class SCOPED_CAPABILITY CLock
-{
-	LOCK m_Lock;
-
-public:
-	CLock() ACQUIRE(m_Lock)
-	{
-		m_Lock = lock_create();
-	}
-
-	~CLock() RELEASE()
-	{
-		lock_destroy(m_Lock);
-	}
-
-	CLock(const CLock &) = delete;
-
-	void Take() ACQUIRE(m_Lock) REQUIRES(!m_Lock) { lock_wait(m_Lock); }
-	void Release() RELEASE() REQUIRES(m_Lock) { lock_unlock(m_Lock); }
-};
-
-class CScopeLock
-{
-	CLock *m_pLock;
-
-public:
-	CScopeLock(CLock *pLock)
-	{
-		m_pLock = pLock;
-		m_pLock->Take();
-	}
-
-	~CScopeLock()
-	{
-		m_pLock->Release();
-	}
-
-	CScopeLock(const CScopeLock &) = delete;
-};
-
 #endif // BASE_TL_THREADING_H

src/engine/client/serverbrowser_http.cpp

@@ -10,6 +10,7 @@
 #include <engine/shared/serverinfo.h>
 #include <engine/storage.h>
 
+#include <base/lock_scope.h>
 #include <base/system.h>
 
 #include <memory>
@@ -133,7 +134,7 @@ void CChooseMaster::Refresh()
 
 void CChooseMaster::CJob::Abort()
 {
-	lock_wait(m_Lock);
+	CLockScope ls(m_Lock);
 	if(m_pHead != nullptr)
 	{
 		m_pHead->Abort();
@@ -143,7 +144,6 @@ void CChooseMaster::CJob::Abort()
 	{
 		m_pGet->Abort();
 	}
-	lock_unlock(m_Lock);
 }
 
 void CChooseMaster::CJob::Run()
@@ -175,9 +175,10 @@ void CChooseMaster::CJob::Run()
 		CHttpRequest *pHead = HttpHead(pUrl).release();
 		pHead->Timeout(Timeout);
 		pHead->LogProgress(HTTPLOG::FAILURE);
-		lock_wait(m_Lock);
-		m_pHead = std::unique_ptr<CHttpRequest>(pHead);
-		lock_unlock(m_Lock);
+		{
+			CLockScope ls(m_Lock);
+			m_pHead = std::unique_ptr<CHttpRequest>(pHead);
+		}
 		IEngine::RunJobBlocking(pHead);
 		if(pHead->State() == HTTP_ABORTED)
 		{
@@ -192,9 +193,10 @@ void CChooseMaster::CJob::Run()
 		CHttpRequest *pGet = HttpGet(pUrl).release();
 		pGet->Timeout(Timeout);
 		pGet->LogProgress(HTTPLOG::FAILURE);
-		lock_wait(m_Lock);
-		m_pGet = std::unique_ptr<CHttpRequest>(pGet);
-		lock_unlock(m_Lock);
+		{
+			CLockScope ls(m_Lock);
+			m_pGet = std::unique_ptr<CHttpRequest>(pGet);
+		}
 		IEngine::RunJobBlocking(pGet);
 		auto Time = std::chrono::duration_cast<std::chrono::milliseconds>(tw::time_get() - StartTime);
 		if(pHead->State() == HTTP_ABORTED)

src/engine/client/updater.cpp

@@ -1,4 +1,5 @@
 #include "updater.h"
+#include <base/lock_scope.h>
 #include <base/system.h>
 #include <engine/client.h>
 #include <engine/engine.h>
@@ -52,10 +53,9 @@ CUpdaterFetchTask::CUpdaterFetchTask(CUpdater *pUpdater, const char *pFile, cons
 
 void CUpdaterFetchTask::OnProgress()
 {
-	lock_wait(m_pUpdater->m_Lock);
+	CLockScope ls(m_pUpdater->m_Lock);
 	str_copy(m_pUpdater->m_aStatus, Dest(), sizeof(m_pUpdater->m_aStatus));
 	m_pUpdater->m_Percent = Progress();
-	lock_unlock(m_pUpdater->m_Lock);
 }
 
 int CUpdaterFetchTask::OnCompletion(int State)
@@ -112,32 +112,26 @@ CUpdater::~CUpdater()
 
 void CUpdater::SetCurrentState(int NewState)
 {
-	lock_wait(m_Lock);
+	CLockScope ls(m_Lock);
 	m_State = NewState;
-	lock_unlock(m_Lock);
 }
 
 int CUpdater::GetCurrentState()
 {
-	lock_wait(m_Lock);
-	int Result = m_State;
-	lock_unlock(m_Lock);
-	return Result;
+	CLockScope ls(m_Lock);
+	return m_State;
 }
 
 void CUpdater::GetCurrentFile(char *pBuf, int BufSize)
 {
-	lock_wait(m_Lock);
+	CLockScope ls(m_Lock);
 	str_copy(pBuf, m_aStatus, BufSize);
-	lock_unlock(m_Lock);
 }
 
 int CUpdater::GetCurrentPercent()
 {
-	lock_wait(m_Lock);
-	int Result = m_Percent;
-	lock_unlock(m_Lock);
-	return Result;
+	CLockScope ls(m_Lock);
+	return m_Percent;
 }
 
 void CUpdater::FetchFile(const char *pFile, const char *pDestPath)

src/engine/client/video.cpp

@@ -4,6 +4,7 @@
 #include <engine/shared/config.h>
 #include <engine/storage.h>
 
+#include <base/lock_scope.h>
 #include <engine/client/graphics_threaded.h>
 #include <engine/sound.h>
 
@@ -467,10 +468,11 @@ void CVideo::RunAudioThread(size_t ParentThreadIndex, size_t ThreadIndex)
 			{
 				std::unique_lock<std::mutex> LockAudio(pThreadData->m_AudioFillMutex);
 
-				lock_wait(g_WriteLock);
-				m_AudioStream.m_vpFrames[ThreadIndex]->pts = av_rescale_q(pThreadData->m_SampleCountStart, AVRational{1, m_AudioStream.pEnc->sample_rate}, m_AudioStream.pEnc->time_base);
-				WriteFrame(&m_AudioStream, ThreadIndex);
-				lock_unlock(g_WriteLock);
+				{
+					CLockScope ls(g_WriteLock);
+					m_AudioStream.m_vpFrames[ThreadIndex]->pts = av_rescale_q(pThreadData->m_SampleCountStart, AVRational{1, m_AudioStream.pEnc->sample_rate}, m_AudioStream.pEnc->time_base);
+					WriteFrame(&m_AudioStream, ThreadIndex);
+				}
 
 				pThreadData->m_AudioFrameToFill = 0;
 				pThreadData->m_AudioFillCond.notify_all();
@@ -547,10 +549,11 @@ void CVideo::RunVideoThread(size_t ParentThreadIndex, size_t ThreadIndex)
 			}
 			{
 				std::unique_lock<std::mutex> LockVideo(pThreadData->m_VideoFillMutex);
-				lock_wait(g_WriteLock);
-				m_VideoStream.m_vpFrames[ThreadIndex]->pts = (int64_t)m_VideoStream.pEnc->frame_number;
-				WriteFrame(&m_VideoStream, ThreadIndex);
-				lock_unlock(g_WriteLock);
+				{
+					CLockScope ls(g_WriteLock);
+					m_VideoStream.m_vpFrames[ThreadIndex]->pts = (int64_t)m_VideoStream.pEnc->frame_number;
+					WriteFrame(&m_VideoStream, ThreadIndex);
+				}
 
 				pThreadData->m_VideoFrameToFill = 0;
 				pThreadData->m_VideoFillCond.notify_all();

src/engine/server/register.cpp

@@ -1,5 +1,6 @@
 #include "register.h"
 
+#include <base/lock_scope.h>
 #include <base/log.h>
 #include <engine/console.h>
 #include <engine/engine.h>
@@ -265,11 +266,14 @@ void CRegister::CProtocol::SendRegister()
 	FormatUuid(m_pParent->m_ChallengeSecret, aChallengeUuid, sizeof(aChallengeUuid));
 	char aChallengeSecret[64];
 	str_format(aChallengeSecret, sizeof(aChallengeSecret), "%s:%s", aChallengeUuid, ProtocolToString(m_Protocol));
+	int InfoSerial;
+	bool SendInfo;
 
-	lock_wait(m_pShared->m_pGlobal->m_Lock);
-	int InfoSerial = m_pShared->m_pGlobal->m_InfoSerial;
-	bool SendInfo = InfoSerial > m_pShared->m_pGlobal->m_LatestSuccessfulInfoSerial;
-	lock_unlock(m_pShared->m_pGlobal->m_Lock);
+	{
+		CLockScope ls(m_pShared->m_pGlobal->m_Lock);
+		InfoSerial = m_pShared->m_pGlobal->m_InfoSerial;
+		SendInfo = InfoSerial > m_pShared->m_pGlobal->m_LatestSuccessfulInfoSerial;
+	}
 
 	std::unique_ptr<CHttpRequest> pRegister;
 	if(SendInfo)
@@ -299,14 +303,16 @@ void CRegister::CProtocol::SendRegister()
 	pRegister->LogProgress(HTTPLOG::FAILURE);
 	pRegister->IpResolve(ProtocolToIpresolve(m_Protocol));
 
-	lock_wait(m_pShared->m_Lock);
-	if(m_pShared->m_LatestResponseStatus != STATUS_OK)
+	int RequestIndex;
 	{
-		log_info(ProtocolToSystem(m_Protocol), "registering...");
+		CLockScope ls(m_pShared->m_Lock);
+		if(m_pShared->m_LatestResponseStatus != STATUS_OK)
+		{
+			log_info(ProtocolToSystem(m_Protocol), "registering...");
+		}
+		RequestIndex = m_pShared->m_NumTotalRequests;
+		m_pShared->m_NumTotalRequests += 1;
 	}
-	int RequestIndex = m_pShared->m_NumTotalRequests;
-	m_pShared->m_NumTotalRequests += 1;
-	lock_unlock(m_pShared->m_Lock);
 	m_pParent->m_pEngine->AddJob(std::make_shared<CJob>(m_Protocol, m_pParent->m_ServerPort, RequestIndex, InfoSerial, m_pShared, std::move(pRegister)));
 	m_NewChallengeToken = false;
 
@@ -359,7 +365,7 @@ CRegister::CProtocol::CProtocol(CRegister *pParent, int Protocol) :
 
 void CRegister::CProtocol::CheckChallengeStatus()
 {
-	lock_wait(m_pShared->m_Lock);
+	CLockScope ls(m_pShared->m_Lock);
 	// No requests in flight?
 	if(m_pShared->m_LatestResponseIndex == m_pShared->m_NumTotalRequests - 1)
 	{
@@ -378,7 +384,6 @@ void CRegister::CProtocol::CheckChallengeStatus()
 			break;
 		}
 	}
-	lock_unlock(m_pShared->m_Lock);
 }
 
 void CRegister::CProtocol::Update()
@@ -434,41 +439,40 @@ void CRegister::CProtocol::CJob::Run()
 		json_value_free(pJson);
 		return;
 	}
-	lock_wait(m_pShared->m_Lock);
-	if(Status != STATUS_OK || Status != m_pShared->m_LatestResponseStatus)
 	{
-		log_debug(ProtocolToSystem(m_Protocol), "status: %s", (const char *)StatusString);
+		CLockScope ls(m_pShared->m_Lock);
+		if(Status != STATUS_OK || Status != m_pShared->m_LatestResponseStatus)
+		{
+			log_debug(ProtocolToSystem(m_Protocol), "status: %s", (const char *)StatusString);
+		}
+		if(Status == m_pShared->m_LatestResponseStatus && Status == STATUS_NEEDCHALLENGE)
+		{
+			log_error(ProtocolToSystem(m_Protocol), "ERROR: the master server reports that clients can not connect to this server.");
+			log_error(ProtocolToSystem(m_Protocol), "ERROR: configure your firewall/nat to let through udp on port %d.", m_ServerPort);
+		}
+		json_value_free(pJson);
+		if(m_Index > m_pShared->m_LatestResponseIndex)
+		{
+			m_pShared->m_LatestResponseIndex = m_Index;
+			m_pShared->m_LatestResponseStatus = Status;
+		}
 	}
-	if(Status == m_pShared->m_LatestResponseStatus && Status == STATUS_NEEDCHALLENGE)
-	{
-		log_error(ProtocolToSystem(m_Protocol), "ERROR: the master server reports that clients can not connect to this server.");
-		log_error(ProtocolToSystem(m_Protocol), "ERROR: configure your firewall/nat to let through udp on port %d.", m_ServerPort);
-	}
-	json_value_free(pJson);
-	if(m_Index > m_pShared->m_LatestResponseIndex)
-	{
-		m_pShared->m_LatestResponseIndex = m_Index;
-		m_pShared->m_LatestResponseStatus = Status;
-	}
-	lock_unlock(m_pShared->m_Lock);
 	if(Status == STATUS_OK)
 	{
-		lock_wait(m_pShared->m_pGlobal->m_Lock);
+		CLockScope ls(m_pShared->m_pGlobal->m_Lock);
 		if(m_InfoSerial > m_pShared->m_pGlobal->m_LatestSuccessfulInfoSerial)
 		{
 			m_pShared->m_pGlobal->m_LatestSuccessfulInfoSerial = m_InfoSerial;
 		}
-		lock_unlock(m_pShared->m_pGlobal->m_Lock);
 	}
 	else if(Status == STATUS_NEEDINFO)
 	{
-		lock_wait(m_pShared->m_pGlobal->m_Lock);
+		CLockScope ls(m_pShared->m_pGlobal->m_Lock);
 		if(m_InfoSerial == m_pShared->m_pGlobal->m_LatestSuccessfulInfoSerial)
 		{
 			// Tell other requests that they need to send the info again.
 			m_pShared->m_pGlobal->m_LatestSuccessfulInfoSerial -= 1;
 		}
-		lock_unlock(m_pShared->m_pGlobal->m_Lock);
 	}
 }
 
@@ -660,9 +664,10 @@ void CRegister::OnNewInfo(const char *pInfo)
 
 	m_GotServerInfo = true;
 	str_copy(m_aServerInfo, pInfo, sizeof(m_aServerInfo));
-	lock_wait(m_pGlobal->m_Lock);
-	m_pGlobal->m_InfoSerial += 1;
-	lock_unlock(m_pGlobal->m_Lock);
+	{
+		CLockScope ls(m_pGlobal->m_Lock);
+		m_pGlobal->m_InfoSerial += 1;
+	}
 
 	// Immediately send new info if it changes, but at most once per second.
 	int64_t Now = time_get();

src/engine/shared/jobs.cpp

@@ -2,6 +2,8 @@
 /* If you are missing that file, acquire a complete release at teeworlds.com.                */
 #include "jobs.h"
 
+#include <base/lock_scope.h>
+
 IJob::IJob() :
 	m_Status(STATE_PENDING)
 {
@@ -54,15 +56,16 @@ void CJobPool::WorkerThread(void *pUser)
 
 		// fetch job from queue
 		sphore_wait(&pPool->m_Semaphore);
-		lock_wait(pPool->m_Lock);
-		if(pPool->m_pFirstJob)
 		{
-			pJob = pPool->m_pFirstJob;
-			pPool->m_pFirstJob = pPool->m_pFirstJob->m_pNext;
-			if(!pPool->m_pFirstJob)
-				pPool->m_pLastJob = 0;
+			CLockScope ls(pPool->m_Lock);
+			if(pPool->m_pFirstJob)
+			{
+				pJob = pPool->m_pFirstJob;
+				pPool->m_pFirstJob = pPool->m_pFirstJob->m_pNext;
+				if(!pPool->m_pFirstJob)
+					pPool->m_pLastJob = 0;
+			}
 		}
-		lock_unlock(pPool->m_Lock);
 
 		// do the job if we have one
 		if(pJob)
@@ -96,16 +99,16 @@ void CJobPool::Destroy()
 
 void CJobPool::Add(std::shared_ptr<IJob> pJob)
 {
-	lock_wait(m_Lock);
+	{
+		CLockScope ls(m_Lock);
+		// add job to queue
+		if(m_pLastJob)
+			m_pLastJob->m_pNext = pJob;
+		m_pLastJob = std::move(pJob);
+		if(!m_pFirstJob)
+			m_pFirstJob = m_pLastJob;
+	}
 
-	// add job to queue
-	if(m_pLastJob)
-		m_pLastJob->m_pNext = pJob;
-	m_pLastJob = std::move(pJob);
-	if(!m_pFirstJob)
-		m_pFirstJob = m_pLastJob;
-
-	lock_unlock(m_Lock);
 	sphore_signal(&m_Semaphore);
 }