Skip to content

Commit 567cce1

Browse files
committed
Fix a bug with the lock hygiene checking code.
Add an explicit check for recursive locking. This is always OK. P4:8789963
1 parent 166c0a5 commit 567cce1

File tree

1 file changed

+33
-27
lines changed

1 file changed

+33
-27
lines changed

src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.cpp

+33-27
Original file line numberDiff line numberDiff line change
@@ -231,40 +231,46 @@ void LockDebugInfo::AboutToLock( bool bTry )
231231
if ( bTry )
232232
return;
233233

234-
// Taking locks of equal order? This will also be true for recursive locks, which are not allowed for 'short duration' locks.
235-
if ( likely( pTopLock->m_nOrder == m_nOrder ) )
234+
// It's always OK to lock recursively.
235+
//
236+
// (Except for "short duration" locks, which are allowed to
237+
// use a mutex implementation that does not support this.)
238+
if ( !( m_nFlags & k_nFlag_ShortDuration ) )
236239
{
237-
if ( m_nOrder < k_nOrder_ObjectOrTable ) // OK to lock global lock recursively
238-
return;
239-
240-
// Taking multiple object locks? This is allowed under certain circumstances
241-
if ( likely( m_nOrder == k_nOrder_ObjectOrTable ) )
240+
for ( int i = 0 ; i < t.m_nHeldLocks ; ++i )
242241
{
243-
244-
// If we hold the global lock, it's OK
245-
if ( bHoldGlobalLock )
242+
if ( t.m_arHeldLocks[i] == this )
246243
return;
244+
}
245+
}
247246

248-
// If the global lock isn't held, then no more than one
249-
// object lock is allowed, since two different threads
250-
// might take them in different order.
251-
constexpr int k_nObjectFlags = LockDebugInfo::k_nFlag_Connection | LockDebugInfo::k_nFlag_PollGroup;
252-
if (
253-
( ( m_nFlags & k_nObjectFlags ) != 0 )
254-
//|| ( 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.
255-
) {
256-
// We must not already hold any existing object locks (except perhaps this one)
257-
for ( int i = 0 ; i < t.m_nHeldLocks ; ++i )
258-
{
259-
const LockDebugInfo *pOtherLock = t.m_arHeldLocks[ i ];
260-
AssertMsg( pOtherLock == this || !( pOtherLock->m_nFlags & k_nObjectFlags ),
261-
"Taking lock '%s' and then '%s', while not holding the global lock", pOtherLock->m_pszName, m_pszName );
262-
}
263-
}
247+
// Taking multiple object locks? This is allowed under certain circumstances
248+
if ( likely( pTopLock->m_nOrder == m_nOrder && m_nOrder == k_nOrder_ObjectOrTable ) )
249+
{
264250

265-
// Usage is OK if we didn't find any problems above
251+
// If we hold the global lock, it's OK
252+
if ( bHoldGlobalLock )
266253
return;
254+
255+
// If the global lock isn't held, then no more than one
256+
// object lock is allowed, since two different threads
257+
// might take them in different order.
258+
constexpr int k_nObjectFlags = LockDebugInfo::k_nFlag_Connection | LockDebugInfo::k_nFlag_PollGroup;
259+
if (
260+
( ( m_nFlags & k_nObjectFlags ) != 0 )
261+
//|| ( 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.
262+
) {
263+
// We must not already hold any existing object locks (except perhaps this one)
264+
for ( int i = 0 ; i < t.m_nHeldLocks ; ++i )
265+
{
266+
const LockDebugInfo *pOtherLock = t.m_arHeldLocks[ i ];
267+
AssertMsg( pOtherLock == this || !( pOtherLock->m_nFlags & k_nObjectFlags ),
268+
"Taking lock '%s' and then '%s', while not holding the global lock", pOtherLock->m_pszName, m_pszName );
269+
}
267270
}
271+
272+
// Usage is OK if we didn't find any problems above
273+
return;
268274
}
269275

270276
AssertMsg( false, "Taking lock '%s' while already holding lock '%s'", m_pszName, pTopLock->m_pszName );

0 commit comments

Comments
 (0)