AuthorizationManagerImpl: Fix thread group thread blocked on condition_variable

[xiexiaoy]

Summary by CodeRabbit

• Refactor
  • Simplified internal locking code across authorization, session, key, plan-cache and related systems by relying on type deduction for lock objects, preserving behavior.
  • Added optional alternate synchronization primitives selectable via a build flag to support coroutine-based locking; no changes to public APIs or runtime behavior.

[coderabbitai]

Walkthrough

The PR adds conditional compilation to switch several internal mutex/condition_variable members to coroutine-based types when D_USE_CORO_SYNC is defined, and simplifies lock construction sites to use CTAD or decltype-based lock types. No public API signatures were changed.

Changes

Cohort / File(s)	Summary
Authorization Manager src/mongo/db/auth/authorization_manager_impl.h, src/mongo/db/auth/authorization_manager_impl.cpp	Header: Adds #ifdef D_USE_CORO_SYNC guards to use coro::Mutex/coro::ConditionVariable and conditionally include coro_sync.h for _privilegeDocsExistMutex, _cacheMutex, and _fetchPhaseIsReady. Implementation: Replaces explicit stdx::unique_lock<stdx::mutex> with stdx::unique_lock<decltype(AuthorizationManagerImpl::_cacheMutex)> and uses CTAD (stdx::unique_lock lk(...)) for _privilegeDocsExistMutex.
Keys Collection Cache src/mongo/db/keys_collection_cache.h, src/mongo/db/keys_collection_cache.cpp	Header: Adds D_USE_CORO_SYNC conditional to switch _cacheMutex to coro::Mutex and include coro_sync.h. Implementation: Replaces stdx::lock_guard<stdx::mutex> with CTAD stdx::lock_guard in refresh, getKeyById, getKey, and resetCache.
Logical Session Cache src/mongo/db/logical_session_cache_impl.h, src/mongo/db/logical_session_cache_impl.cpp	Header: Adds D_USE_CORO_SYNC guards to use coro::Mutex for _reaperMutex and _cacheMutex and include coro_sync.h. Implementation: Replaces stdx::lock_guard<stdx::mutex> with CTAD stdx::lock_guard across methods (promote, size, _reap, _refresh, endSessions, getStats, _addToCache, listIds, peekCached).
Plan Cache src/mongo/db/query/plan_cache.h, src/mongo/db/query/plan_cache.cpp	Header: Adds D_USE_CORO_SYNC conditional to swap _cacheMutex to coro::Mutex and include coro_sync.h. Implementation: Replaces explicit stdx::lock_guard<stdx::mutex> with CTAD stdx::lock_guard across PlanCache member functions.
Time Proof Service src/mongo/db/time_proof_service.h, src/mongo/db/time_proof_service.cpp	Header: Adds D_USE_CORO_SYNC conditional includes and switches _cacheMutex to coro::Mutex when defined. Implementation: Replaces stdx::lock_guard<stdx::mutex> with CTAD stdx::lock_guard in two locations.
Validate Command src/mongo/db/commands/validate.cpp	Adds D_USE_CORO_SYNC guards to swap stdx::mutex/stdx::condition_variable for coroutine equivalents, conditionally includes coro_sync.h, and simplifies a couple of lock constructions to CTAD.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Check consistency of #ifdef D_USE_CORO_SYNC guards and paired #include changes across headers.
• Verify CTAD usage for stdx::lock_guard and stdx::unique_lock compiles with both stdx::mutex and coro::Mutex.
• Inspect decltype(AuthorizationManagerImpl::_cacheMutex) usage for correct lookup and accessibility in the translation unit.
• Ensure any coro::ConditionVariable members are properly guarded and initialized where added.

Poem

🐇 I hopped through headers, nimble and bright,
Swapped locks for coros in dim compiler light.
CTAD hummed softly, “just deduce, don’t state,”
Guards wrapped in macros made choices innate.
A small rabbit cheers this tidy sync flight.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title focuses on a specific fix for AuthorizationManagerImpl and condition_variable blocking, but the changeset encompasses much broader refactoring across 10+ files involving mutex abstraction and coroutine synchronization primitives.	Consider a title that better reflects the full scope of changes, such as 'Introduce conditional coroutine-based synchronization for multiple cache and lock implementations' or clarify if this PR should be split into smaller, focused changes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.90% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe06230 and 49db161.

📒 Files selected for processing (11)

• src/mongo/db/auth/authorization_manager_impl.cpp (2 hunks)
• src/mongo/db/auth/authorization_manager_impl.h (4 hunks)
• src/mongo/db/commands/validate.cpp (5 hunks)
• src/mongo/db/keys_collection_cache.cpp (5 hunks)
• src/mongo/db/keys_collection_cache.h (2 hunks)
• src/mongo/db/logical_session_cache_impl.cpp (15 hunks)
• src/mongo/db/logical_session_cache_impl.h (2 hunks)
• src/mongo/db/query/plan_cache.cpp (7 hunks)
• src/mongo/db/query/plan_cache.h (2 hunks)
• src/mongo/db/time_proof_service.cpp (2 hunks)
• src/mongo/db/time_proof_service.h (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (6)

• src/mongo/db/query/plan_cache.h
• src/mongo/db/logical_session_cache_impl.cpp
• src/mongo/db/logical_session_cache_impl.h
• src/mongo/db/query/plan_cache.cpp
• src/mongo/db/time_proof_service.cpp
• src/mongo/db/keys_collection_cache.h

🧰 Additional context used

🧬 Code graph analysis (1)

src/mongo/db/auth/authorization_manager_impl.h (1)

src/mongo/db/coro_sync.h (3)

• coro (30-115)
• Mutex (42-55)
• ConditionVariable (57-114)

🔇 Additional comments (8)

src/mongo/db/keys_collection_cache.cpp (1)

48-48: LGTM! Clean adoption of CTAD for lock guards.

The migration to CTAD-based stdx::lock_guard correctly adapts to the conditionally-typed _cacheMutex (either stdx::mutex or coro::Mutex based on D_USE_CORO_SYNC). This approach maintains identical locking semantics while enabling the conditional synchronization strategy.

Also applies to: 65-65, 88-88, 105-105, 120-120

src/mongo/db/auth/authorization_manager_impl.h (2)

204-208: LGTM! Conditional synchronization primitives correctly applied.

The conditional compilation cleanly switches between stdx and coro synchronization primitives for internal members. Both paths provide compatible interfaces (lock/unlock for mutexes, wait/notify for condition variables), ensuring the implementation works correctly under both configurations.

Also applies to: 244-259

---

52-57: No changes needed—the current include ordering is correct.

The placement of mongo/stdx/functional.h outside the conditional block is intentional and consistent across the codebase. The search results show that in files like thread_pool.h, queue.h, and clock_source.h, functional.h is always included unconditionally, even when synchronization primitives (condition_variable.h, mutex.h) are conditionally included. This pattern indicates that functional.h has no dependency on the sync primitives and should remain separate from the #ifndef D_USE_CORO_SYNC block.

src/mongo/db/commands/validate.cpp (2)

46-49: LGTM! Conditional synchronization primitives properly introduced.

The conditional compilation correctly switches the global validation synchronization primitives between stdx and coro types based on D_USE_CORO_SYNC.

Also applies to: 60-69

---

174-174: No issues found—compatibility is correctly handled.

The code uses conditional compilation (#ifndef D_USE_CORO_SYNC) to ensure _validationNotifier and _validationMutex are always paired as the same family of types: either both stdx:: or both coro::. The waitForConditionOrInterrupt() method has proper overloads in operation_context.h (lines 168–175) supporting both type families, with the standard types delegating to the coro implementation via safe reinterpret_casts. Line 191 is a separate context using lock_guard for notify_all(), which is a method on the condition variable itself—not a call to waitForConditionOrInterrupt.

Likely an incorrect or invalid review comment.

src/mongo/db/time_proof_service.h (1)

34-39: LGTM! Straightforward conditional mutex type.

The conditional compilation cleanly switches _cacheMutex between stdx::mutex and coro::Mutex. The implementation is correct and maintains the same locking interface.

Also applies to: 97-101

src/mongo/db/auth/authorization_manager_impl.cpp (2)

249-249: LGTM! Correct use of decltype for type adaptation.

Using decltype(AuthorizationManagerImpl::_cacheMutex) ensures CacheGuard::_lock correctly adapts to the conditionally-typed _cacheMutex, maintaining compatibility with both stdx::mutex and coro::Mutex.

---

327-327: No compatibility issue found—code is correct as-is.

stdx::unique_lock is a direct alias for std::unique_lock, and it works correctly with the conditionally-typed _privilegeDocsExistMutex. When D_USE_CORO_SYNC is defined, the mutex becomes coro::Mutex, which provides the required lock(), unlock(), and try_lock() methods. The resulting std::unique_lock<coro::Mutex> is fully compatible with coro::ConditionVariable::wait which expects std::unique_lock&. Template deduction automatically handles both conditional mutex types correctly.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/mongo/db/logical_session_cache_impl.h (1)

42-44: Note: Conditional compilation pattern differs from other files.

This file uses #ifdef D_USE_CORO_SYNC while other files use #ifndef D_USE_CORO_SYNC. Both are logically correct, but the inconsistency may reduce maintainability.

Consider aligning with the pattern used in other files for consistency:

-#ifdef D_USE_CORO_SYNC
+#ifndef D_USE_CORO_SYNC
+    mutable stdx::mutex _reaperMutex;
+#else
 #include "mongo/db/coro_sync.h"
-#endif
-
-namespace mongo {
-...
-#ifndef D_USE_CORO_SYNC
-    mutable stdx::mutex _reaperMutex;
-#else
     mutable coro::Mutex _reaperMutex;
 #endif

Also applies to: 165-169, 172-176

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 06ebb54 and fe06230.

📒 Files selected for processing (10)

• src/mongo/db/auth/authorization_manager_impl.cpp (2 hunks)
• src/mongo/db/auth/authorization_manager_impl.h (4 hunks)
• src/mongo/db/keys_collection_cache.cpp (5 hunks)
• src/mongo/db/keys_collection_cache.h (2 hunks)
• src/mongo/db/logical_session_cache_impl.cpp (15 hunks)
• src/mongo/db/logical_session_cache_impl.h (2 hunks)
• src/mongo/db/query/plan_cache.cpp (7 hunks)
• src/mongo/db/query/plan_cache.h (2 hunks)
• src/mongo/db/time_proof_service.cpp (2 hunks)
• src/mongo/db/time_proof_service.h (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (4)

src/mongo/db/query/plan_cache.cpp (1)

src/mongo/db/logical_session_cache_impl.cpp (2)

• size (169-172)
• size (169-169)

src/mongo/db/auth/authorization_manager_impl.h (1)

src/mongo/db/coro_sync.h (3)

• coro (30-115)
• Mutex (42-55)
• ConditionVariable (57-114)

src/mongo/db/logical_session_cache_impl.h (1)

src/mongo/db/coro_sync.h (2)

• coro (30-115)
• Mutex (42-55)

src/mongo/db/query/plan_cache.h (1)

src/mongo/db/coro_sync.h (2)

• coro (30-115)
• Mutex (42-55)

🔇 Additional comments (11)

src/mongo/db/keys_collection_cache.h (1)

37-41: LGTM! Conditional compilation setup is clean.

The conditional includes and mutex type switching are well-structured and maintain API compatibility.

Also applies to: 75-79

src/mongo/db/keys_collection_cache.cpp (1)

48-48: LGTM! CTAD usage simplifies lock declarations.

The switch to Class Template Argument Deduction for lock_guard is appropriate and works seamlessly with both stdx::mutex and coro::Mutex types.

Also applies to: 65-65, 88-88, 105-105, 120-120

src/mongo/db/auth/authorization_manager_impl.h (1)

52-57: LGTM! Conditional synchronization primitives properly configured.

The dual-path implementation correctly switches between stdx and coro synchronization primitives while preserving the public API.

Also applies to: 204-208, 244-259

src/mongo/db/auth/authorization_manager_impl.cpp (2)

249-249: LGTM! Proper use of decltype for member variable.

Using decltype(AuthorizationManagerImpl::_cacheMutex) is the correct approach for deducing the lock type in a member variable declaration, as CTAD doesn't apply to member variables.

---

327-327: LGTM! CTAD simplifies local lock declaration.

The type deduction for unique_lock is appropriate for local variables and works with both mutex types.

src/mongo/db/query/plan_cache.cpp (1)

750-750: LGTM! Consistent CTAD usage across all locking sites.

The migration to type-deduced lock_guard is systematic and correct across all methods.

Also applies to: 765-765, 785-785, 802-802, 807-807, 823-823, 837-837, 849-849, 854-854

src/mongo/db/time_proof_service.cpp (1)

59-59: LGTM! CTAD usage is consistent.

The simplified lock declarations align with the broader PR pattern and work correctly with both mutex types.

Also applies to: 84-84

src/mongo/db/query/plan_cache.h (1)

42-46: LGTM! Conditional compilation is properly structured.

The mutex type switching follows the established pattern and maintains a clean separation between the two synchronization implementations.

Also applies to: 430-434

src/mongo/db/logical_session_cache_impl.cpp (1)

95-95: LGTM: Clean adoption of CTAD for lock_guard.

The removal of explicit template parameters throughout the file is a correct and beneficial modernization. Using Class Template Argument Deduction (CTAD) simplifies the syntax while maintaining identical semantics. These changes work correctly with the conditional mutex types introduced elsewhere in the PR (stdx::mutex vs. coro::Mutex), as both provide compatible lock interfaces.

Also applies to: 170-170, 198-198, 229-229, 233-233, 242-242, 254-254, 269-269, 302-302, 313-313, 346-346, 354-354, 367-367, 397-397, 403-403, 408-408, 414-414, 423-423, 434-434, 447-447

src/mongo/db/time_proof_service.h (2)

34-39: Conditional compilation pattern looks correct.

The conditional include structure properly switches between stdx and coro synchronization headers based on the D_USE_CORO_SYNC macro, maintaining consistency with similar changes across the PR.

---

97-101: Conditional mutex declaration is correct; coro::Mutex is the primary tested implementation.

The D_USE_CORO_SYNC flag is unconditionally defined in service_context.h, making coro::Mutex the standard code path throughout the codebase. The conditional pattern used in time_proof_service.h follows the established convention across multiple components (authorization_manager_impl, session_catalog, plan_cache, etc.). Since coro::Mutex is the active implementation in normal builds, its compatibility with stdx::lock_guard is already verified through existing test coverage.