[SERVER-67383] Layering violation in the lock manager Created: 20/Jun/22 Updated: 29/Oct/23 Resolved: 16/Sep/22 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 6.2.0-rc0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Kaloian Manassiev | Assignee: | Gregory Noma |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | catalog, techdebt | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Operating System: | ALL | ||||||||
| Sprint: | Execution Team 2022-09-19, Execution Team 2022-10-03 | ||||||||
| Participants: | |||||||||
| Description |
|
The RAII classes in d_concurrency.h/.cpp are strictly part of the lock manager. As such, they should be completely decoupled from the database/collection catalog and should not know about UUIDs or anything else that is strictly catalog. This is not the case currently since the CollectionLock object at the very least is aware of the collection catalog which is a layering violation that can lead to bugs such as UPDATE: Proposed solution is to add a lightweight RAII type on top of the CollectionLock use. The selection to use the CollectionLock in the first place, over AutoGetCollection*, may imply that sharding/view/storage checks are not desired for the use cases. So we'd have to investigate what works. |
| Comments |
| Comment by Githook User [ 16/Sep/22 ] |
|
Author: {'name': 'Gregory Noma', 'email': 'gregory.noma@gmail.com', 'username': 'gregorynoma'}Message: |
| Comment by Githook User [ 16/Sep/22 ] |
|
Author: {'name': 'Gregory Noma', 'email': 'gregory.noma@gmail.com', 'username': 'gregorynoma'}Message: |
| Comment by Dianna Hohensee (Inactive) [ 02/Aug/22 ] |
|
Spoke with Kal, and updated the ticket description with the proposed solution. |
| Comment by Kaloian Manassiev [ 28/Jul/22 ] |
|
d_concurrency is a library strictly about locking RAII and the lock manager locks by namespaces, not by UUID. The UUID to namespace conversion is something that happens in the layer above the lock manager. All that I am asking is that d_concurrency is left to not rely on the CollectionCatalog. If there is any site that needs to lock by UUID, it should either be using AutoGetCollection, which already does that conversion or there should be something between AutoGetCollection and d_concurrency which does it and is not linked in d_concurrency. |
| Comment by Dianna Hohensee (Inactive) [ 27/Jul/22 ] |
|
kaloian.manassiev@mongodb.com let me know if there are any dependencies of which I'm unaware on this work. I think we would need to see some significant gains to motivate decoupling this, to counter the loss of ease of use. |
| Comment by Dianna Hohensee (Inactive) [ 27/Jul/22 ] |
|
Conceptually, the LockManager treats namespaces as unique identifiers. Since commands are run targeting specific UUIDs, there must be a way to ascertain that the namespace still matches the UUID requested to be locked. The CollectionLock constructor takes nssOrUUID as input and handles the UUID/namespace resolution internally without the caller needing to be aware. There is a pile of CollectionLock call sites all throughout the codebase, most of which take namespace, but maybe a dozen of which take UUID. I don't see any improvement in simply adding a layer between CollectionLock and all the existing call sites that currently create CollectionLocks. CollectionLock would be unusable without the wrapper using the CollectionCatalog. However, we could selectively specially wrap the UUID callers only. This would effectively undo the code cleanup of I don't see any pressing reasons to separate out the CollectionCatalog from the d_concurrency files. The code appears cleaner and easier to understand and use combined. And facilitates future changes towards more UUID usage. I'm inclined to leave the code as is, unless there's good reason to change it. All callers taking collection level locks directly undoubtedly proceed to access the collection through the CollectionCatalog, so the linking will occur regardless. |