[SERVER-69915] Consider calling getOwned for object_owned idl type Created: 22/Sep/22  Updated: 20/Mar/23

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

Type: Improvement Priority: Major - P3
Reporter: Randolph Tan Assignee: Backlog - Service Architecture
Resolution: Unresolved Votes: 0
Labels: lowcontext
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-51846 IDL parser should create owned copy o... Backlog
is related to SERVER-59841 Consider whether ownership should be ... Backlog
Assigned Teams:
Service Arch
Participants:

 Description   

when generating the constructor.

Example:
https://github.com/mongodb/mongo/blob/eac5afb151fc64740763316c128a46c8c664d37e/src/mongo/db/s/config/set_cluster_parameter_coordinator_document.idl#L58

Generates code looks like this:

SetClusterParameterCoordinatorDocument::SetClusterParameterCoordinatorDocument(mongo::BSONObj parameter) : _parameter(std::move(parameter)), _hasParameter(true) {
    // Used for initialization only
}

Instead of

_parameter(std::move(parameter))

consider doing this instead:

_parameter(parameter.getOwned())



 Comments   
Comment by Billy Donahue [ 23/Sep/22 ]

I see. Thanks Max.

I'm wondering how the "owned" guarantee should be enforced now.

The owned-ness is dynamic, which is something that keeps causing bugs like this one.
There's no type for an "owned BSONObj". If there was we'd just accept it here and not BSONObj.

Taking ownership is basically free if the object is already owned.
If the object is not already owned, we have to expensively copy it out into a new object.

Suppose there was an "OwnedBSONObj" type. Obviously it would have a constructor that takes BSONObj.
But would that constructor be implicit or explicit? I'm guessing explicit, because it's an expensive operation, like string_view to string.

So for these setters and constructors, it's not clear that an unowned BSONObj should be accepted at all.
It almost feels like an invariant violation. Maybe that's too extreme.

Comment by Max Hirschhorn [ 23/Sep/22 ]

The object_owned type is meant to guarantee the BSONObj underlying the struct member is an owned BSONObj. That is, the IDL struct participates in keeping the underlying BSONObj alive. However, getOwned() only ends up being called when parsing and not in the constructor. This means the constructor + setters on the IDL struct allow the members to be unowned BSONObj still.

void setParameter(mongo::BSONObj value) {  _parameter = std::move(value); _hasParameter = true; }

else if (fieldName == kParameterFieldName) {
    if (MONGO_likely(ctxt.checkAndAssertType(element, Object))) {
        if (MONGO_unlikely(usedFields[kParameterBit])) {
            ctxt.throwDuplicateField(element);
        }
 
        usedFields.set(kParameterBit);
 
        _hasParameter = true;
        const BSONObj localObject = element.Obj();
        _parameter = BSONObj::getOwned(localObject);
    }
}

Comment by Billy Donahue [ 22/Sep/22 ]

Why would getOwned() be better than a std::move ?

Asking for real. I don't know what the "object_owned" type means exactly.

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