[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:
Related
related to SERVER-67382 Race condition in the UUID <-> NSS re... Closed
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 SERVER-67382.

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: SERVER-67383 Track resource names using `ResourceCatalog`
Branch: master
https://github.com/mongodb/mongo/commit/bfd35c950c9863ef08b8f7adbdf760820ef4da96

Comment by Githook User [ 16/Sep/22 ]

Author:

{'name': 'Gregory Noma', 'email': 'gregory.noma@gmail.com', 'username': 'gregorynoma'}

Message: SERVER-67383 Use `CollectionNamespaceOrUUIDLock` to lock collections via UUID
Branch: master
https://github.com/mongodb/mongo/commit/ec54f761d043e56cf484de864a89c3645223f878

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 SERVER-42527.

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.

Generated at Thu Feb 08 06:07:59 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.