[SERVER-68942] Use type-checking to ensure correct DB locks are held Created: 18/Aug/22  Updated: 14/Aug/23

Status: Backlog
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Matthew Saltz (Inactive) Assignee: Backlog - Storage Execution Team
Resolution: Unresolved Votes: 0
Labels: techdebt
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
duplicates SERVER-60318 (Investigation) Create something simi... Backlog
Assigned Teams:
Storage Execution
Participants:

 Description   

I've noticed we have a lot of comments on functions that say things like "The collection IX lock must be held before calling this function", and then inside the function body, we will invariant that the correct lock is held. For example, see comments for DatabaseHolder::openDb, and the corresponding invariant.

This was true in some sharding code as well, and in the past the comments have gotten stale, and the invariant may or may not be kept up-to-date. (The invariant probably stays up-to-date.)

I'm wondering if we could instead use a construct akin to WithLock, but that works with database/collection locks instead.

So DatabaseHolder::openDb would look more like

    virtual Database* openDb(OperationContext* opCtx,
                                                 const DatabaseName& dbName,
                                                 WithDbLock::X,
                                                 bool* justCreated = nullptr) = 0;

and callers might do something like

dbHolder->openDb(
        opCtx, 
        dbName, 
        WithDbLock::X(opCtx->lockState(), dbName));

The constructor of WithDbLock::X could have an invariant that the correct lock is held for the database. So it's kind of a hybrid of doing some type-checking as well as some runtime checks (which I think are required since you want to check that the lock is held for the correct database).

You could also have constructors of WithDbLock::X (and the other variants for other lock types) also be implicitly convertible from DBLock so that use cases like this one could pass the lock object directly, like:

Lock::DBLock dbLock(opCtx, dbName, MODE_X);
dbHolder->openDb(
        opCtx, 
        dbName, 
        dbLock);

To do this properly so that you catch invalid lock types at compile time might require some extra work in making different DBLock types like DBLock::X, DBLock::IX that are usable instead of passing flags. E.g.:

Lock::DBLock::X dbLock(opCtx, dbName);
dbHolder->openDb(
        opCtx, 
        dbName, 
        dbLock);

Then you could have the correct lock types be implicitly convertible from others so that the semantics worked correctly. E.g. a WithDbLock::IX would be constructible from a DBLock::X and a DBLock::IX, but not from DBLock::S.

One downside is I'm not sure how to enforce that the caller would pass in the name for the correct database, but that error feels hard to make.

I'm not sure whether on the whole this would prevent bugs or not, but it might help a little with documentation at the very least.



 Comments   
Comment by Dianna Hohensee (Inactive) [ 23/Aug/22 ]

This is the same idea as SERVER-60318, but more brainstorming on the subject.

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