Adjust our lock hygiene checking framework

Allow multiple ShortDurationLocks to be taken, as long as they are taken in
a certain order.

P4:8765422

src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp

@@ -427,7 +427,7 @@ CSteamNetworkingSockets::CSteamNetworkingSockets( CSteamNetworkingUtils *pSteamN
 #endif
 , m_bEverTriedToGetCert( false )
 , m_bEverGotCert( false )
-, m_mutexPendingCallbacks( "pending_callbacks" )
+, m_mutexPendingCallbacks( "pending_callbacks", LockDebugInfo::k_nOrder_Max ) // Never take another lock while holding this
 {
 	m_connectionConfig.Init( nullptr );
 	InternalClearIdentity();

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp

@@ -164,7 +164,7 @@ void CSteamNetworkingMessage::Unlink()
 	UnlinkFromQueue( &CSteamNetworkingMessage::m_linksSecondaryQueue );
 }
 
-ShortDurationLock g_lockAllRecvMessageQueues( "all_recv_msg_queue" );
+ShortDurationLock g_lockAllRecvMessageQueues( "all_recv_msg_queue", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this
 
 void SteamNetworkingMessageQueue::AssertLockHeld() const
 {
@@ -638,7 +638,7 @@ CSteamNetworkConnectionBase::~CSteamNetworkConnectionBase()
 }
 
 static std::vector<CSteamNetworkConnectionBase *> s_vecPendingDeleteConnections;
-static ShortDurationLock s_lockPendingDeleteConnections( "connection_delete_queue" );
+static ShortDurationLock s_lockPendingDeleteConnections( "connection_delete_queue", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this
 
 void CSteamNetworkConnectionBase::ConnectionQueueDestroy()
 {

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h

@@ -248,7 +248,7 @@ private:
 
 class CSteamNetworkPollGroup;
 struct PollGroupLock : Lock<RecursiveTimedMutexImpl> {
-	PollGroupLock() : Lock<RecursiveTimedMutexImpl>( "pollgroup", LockDebugInfo::k_nFlag_PollGroup ) {}
+	PollGroupLock() : Lock<RecursiveTimedMutexImpl>( "pollgroup", LockDebugInfo::k_nFlag_PollGroup, LockDebugInfo::k_nOrder_ObjectOrTable ) {}
 };
 using PollGroupScopeLock = ScopeLock<PollGroupLock>;
 
@@ -339,7 +339,7 @@ protected:
 /////////////////////////////////////////////////////////////////////////////
 
 struct ConnectionLock : Lock<RecursiveTimedMutexImpl> {
-	ConnectionLock() : Lock<RecursiveTimedMutexImpl>( "connection", LockDebugInfo::k_nFlag_Connection ) {}
+	ConnectionLock() : Lock<RecursiveTimedMutexImpl>( "connection", LockDebugInfo::k_nFlag_Connection, LockDebugInfo::k_nOrder_ObjectOrTable ) {}
 };
 struct ConnectionScopeLock : ScopeLock<ConnectionLock>
 {
@@ -1115,7 +1115,7 @@ extern CUtlHashMap<int, CSteamNetworkPollGroup *, std::equal_to<int>, Identity<i
 
 // All of the tables above are projected by the same lock, since we expect to only access it briefly
 struct TableLock : Lock<RecursiveTimedMutexImpl> {
-	TableLock() : Lock<RecursiveTimedMutexImpl>( "table", LockDebugInfo::k_nFlag_Table ) {}
+	TableLock() : Lock<RecursiveTimedMutexImpl>( "table", LockDebugInfo::k_nFlag_Table, LockDebugInfo::k_nOrder_ObjectOrTable ) {}
 }; 
 using TableScopeLock = ScopeLock<TableLock>;
 extern TableLock g_tables_lock;

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.cpp

@@ -83,7 +83,7 @@ static void FlushSystemSpew();
 int g_cbUDPSocketBufferSize = 256*1024;
 
 /// Global lock for all local data structures
-static Lock<RecursiveTimedMutexImpl> s_mutexGlobalLock( "global", 0 );
+static Lock<RecursiveTimedMutexImpl> s_mutexGlobalLock( "global", 0, LockDebugInfo::k_nOrder_Global );
 
 #if STEAMNETWORKINGSOCKETS_LOCK_DEBUG_LEVEL > 0
 
@@ -208,37 +208,50 @@ void LockDebugInfo::AboutToLock( bool bTry )
 	{
 		// Remember when we started trying to lock
 		t.m_usecOuterLockStartTime = SteamNetworkingSockets_GetLocalTimestamp();
+		return;
 	}
-	else
+
+	// We already hold a lock.  Check for taking locks in such a way
+	// that might lead to deadlocks.
+	const LockDebugInfo *pTopLock = t.m_arHeldLocks[ t.m_nHeldLocks-1 ];
+
+	// Taking locks in increasing order is always allowed
+	if ( likely( pTopLock->m_nOrder < m_nOrder ) )
+		return;
+
+	// Global lock *must* always be the outermost lock.  (It is legal to take other locks in
+	// between and then lock the global lock recursively.)
+	const bool bHoldGlobalLock = t.m_arHeldLocks[ 0 ] == &s_mutexGlobalLock;
+	AssertMsg(
+		bHoldGlobalLock || this != &s_mutexGlobalLock,
+		"Taking global lock while already holding lock '%s'", t.m_arHeldLocks[ 0 ]->m_pszName
+	);
+
+	// If they are only "trying", we allow out-of-order behaviour.
+	if ( bTry )
+		return;
+
+	// Taking locks of equal order?  This will also be true for recursive locks, which are not allowed for 'short duration' locks.
+	if ( likely( pTopLock->m_nOrder == m_nOrder ) )
 	{
+		if ( m_nOrder < k_nOrder_ObjectOrTable ) // OK to lock global lock recursively
+			return;
 
-		// We already hold a lock.  Make sure it's legal for us to take another!
-
-		// Global lock *must* always be the outermost lock.  (It is legal to take other locks in
-		// between and then lock the global lock recursively.)
-		const bool bHoldGlobalLock = t.m_arHeldLocks[ 0 ] == &s_mutexGlobalLock;
-		AssertMsg(
-			bHoldGlobalLock || this != &s_mutexGlobalLock,
-			"Taking global lock while already holding lock '%s'", t.m_arHeldLocks[ 0 ]->m_pszName
-		);
-
-		// Check for taking locks in such a way that might lead to deadlocks.
-		// If they are only "trying", then we do allow out of order behaviour.
-		if ( !bTry )
+		// Taking multiple object locks?  This is allowed under certain circumstances
+		if ( likely( m_nOrder == k_nOrder_ObjectOrTable ) )
 		{
-			const LockDebugInfo *pTopLock = t.m_arHeldLocks[ t.m_nHeldLocks-1 ];
 
-			// Once we take a "short duration" lock, we must not
-			// take any additional locks!  (Including a recursive lock.)
-			AssertMsg( !( pTopLock->m_nFlags & LockDebugInfo::k_nFlag_ShortDuration ), "Taking lock '%s' while already holding lock '%s'", m_pszName, pTopLock->m_pszName );
+			// If we hold the global lock, it's OK
+			if ( bHoldGlobalLock )
+				return;
 
 			// If the global lock isn't held, then no more than one
 			// object lock is allowed, since two different threads
 			// might take them in different order.
 			constexpr int k_nObjectFlags = LockDebugInfo::k_nFlag_Connection | LockDebugInfo::k_nFlag_PollGroup;
 			if (
-				( !bHoldGlobalLock && ( m_nFlags & k_nObjectFlags ) != 0 )
-				//|| ( m_nFlags & k_nFlag_Table ) // We actually do this in one place when we know it's OK.  Not wirth it right now to get this situation exempted from the checking.
+				( ( m_nFlags & k_nObjectFlags ) != 0 )
+				//|| ( m_nFlags & k_nFlag_Table ) // We actually do this in one place when we know it's OK.  Not worth it right now to get this situation exempted from the checking.
 			) {
 				// We must not already hold any existing object locks (except perhaps this one)
 				for ( int i = 0 ; i < t.m_nHeldLocks ; ++i )
@@ -248,8 +261,13 @@ void LockDebugInfo::AboutToLock( bool bTry )
 						"Taking lock '%s' and then '%s', while not holding the global lock", pOtherLock->m_pszName, m_pszName );
 				}
 			}
+
+			// Usage is OK if we didn't find any problems above
+			return;
 		}
 	}
+
+	AssertMsg( false, "Taking lock '%s' while already holding lock '%s'", m_pszName, pTopLock->m_pszName );
 }
 
 void LockDebugInfo::OnLocked( const char *pszTag )
@@ -457,7 +475,7 @@ static volatile bool s_bManualPollMode;
 //
 /////////////////////////////////////////////////////////////////////////////
 
-ShortDurationLock s_lockTaskQueue( "TaskQueue" );
+ShortDurationLock s_lockTaskQueue( "TaskQueue", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this
 
 CTaskTarget::~CTaskTarget()
 {
@@ -3461,7 +3479,7 @@ void CSharedSocket::RemoteHost::Close()
 //
 /////////////////////////////////////////////////////////////////////////////
 
-static ShortDurationLock s_systemSpewLock( "SystemSpew" );
+static ShortDurationLock s_systemSpewLock( "SystemSpew", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this
 SteamNetworkingMicroseconds g_usecLastRateLimitSpew;
 int g_nRateLimitSpewCount;
 ESteamNetworkingSocketsDebugOutputType g_eAppSpewLevel = k_ESteamNetworkingSocketsDebugOutputType_Msg; // Option selected by app

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.h

@@ -452,8 +452,31 @@ struct LockDebugInfo
 	static constexpr int k_nFlag_PollGroup = (1<<2);
 	static constexpr int k_nFlag_Table = (1<<4);
 
+	// When multiple locks are taken, there is potential for a deadlock.
+	// Locks taken in increasing order is always allowed, in decreasing
+	// order is never allowed.  In certain situations we allow locks
+	// of the same order to be taken.
+	enum {
+
+		// Global lock must always be taken first
+		k_nOrder_Global,
+
+		// Object locks (table, connection, poll group) can be taken in any
+		// order while holding the global lock.  Otherwise, you may not take
+		// more than one.
+		k_nOrder_ObjectOrTable,
+
+		// We might need to take a lock to queue callbacks while holding this.
+		k_nOrder_NetworkAndCachedRoutes,
+
+		// Short duration locks are usually the lowest priority and we usually
+		// should not take more than one of these at a time
+		k_nOrder_Max
+	};
+
 	const char *const m_pszName;
 	const int m_nFlags;
+	const int m_nOrder;
 
 	#if STEAMNETWORKINGSOCKETS_LOCK_DEBUG_LEVEL > 0
 		void _AssertHeldByCurrentThread( const char *pszFile, int line, const char *pszTag = nullptr ) const;
@@ -462,7 +485,7 @@ struct LockDebugInfo
 	#endif
 
 protected:
-	LockDebugInfo( const char *pszName, int nFlags ) : m_pszName( pszName ), m_nFlags( nFlags ) {}
+	LockDebugInfo( const char *pszName, int nFlags, int nOrder ) : m_pszName( pszName ), m_nFlags( nFlags ), m_nOrder( nOrder ) {}
 
 	#if STEAMNETWORKINGSOCKETS_LOCK_DEBUG_LEVEL > 0
 		void AboutToLock( bool bTry );
@@ -480,7 +503,7 @@ protected:
 template<typename TMutexImpl >
 struct Lock : LockDebugInfo
 {
-	inline Lock( const char *pszName, int nFlags ) : LockDebugInfo( pszName, nFlags ) {}
+	inline Lock( const char *pszName, int nFlags, int nOrder ) : LockDebugInfo( pszName, nFlags, nOrder ) {}
 	inline void lock( const char *pszTag = nullptr )
 	{
 		LockDebugInfo::AboutToLock( false );
@@ -572,11 +595,11 @@ private:
 // A very simple lock to protect short accesses to a small set of data.
 // Used when:
 // - We hold the lock for a brief period.
-// - We don't need to take any additional locks while already holding this one.
-//   (Including this lock -- e.g. we don't need to lock recursively.)
+// - We don't need to lock recursively.
+// - For most locks, we also don't allow taking of any other locks.  (In a few cases we use the order to mark which are allowed.)
 struct ShortDurationLock : Lock<ShortDurationMutexImpl>
 {
-	ShortDurationLock( const char *pszName ) : Lock<ShortDurationMutexImpl>( pszName, k_nFlag_ShortDuration ) {}
+	ShortDurationLock( const char *pszName, int nOrder ) : Lock<ShortDurationMutexImpl>( pszName, k_nFlag_ShortDuration, nOrder ) {}
 };
 using ShortDurationScopeLock = ScopeLock<ShortDurationLock>;
 

src/steamnetworkingsockets/steamnetworkingsockets_thinker.cpp

@@ -62,7 +62,7 @@ IThinker::~IThinker()
 struct ShortDurationLock { inline void lock() {}; inline void unlock() {}; };
 static ShortDurationLock s_mutexThinkerTable;
 #else
-static ShortDurationLock s_mutexThinkerTable( "thinker" );
+static ShortDurationLock s_mutexThinkerTable( "thinker", ShortDurationLock::k_nOrder_Max ); // We do sometimes take another lock, but it is always a "try"
 #endif
 
 // Base class isn't lockable