[SERVER-76295] Can TypedCommand parse request in owned manner? Created: 19/Apr/23  Updated: 21/Apr/23  Resolved: 21/Apr/23

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

Type: Bug Priority: Major - P3
Reporter: Sandeep Dhoot Assignee: Backlog - Service Architecture
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Assigned Teams:
Service Arch
Operating System: ALL
Participants:

 Description   

When I use TypedCommand, any BSONObjs in my request struct are not parsed in owned manner (e.g. here). This means that I cannot hold onto these objects after the command finishes running. I need to explicitly call `getOwned` on these objects to make them outlive command execution. While I can do this, this is brittle, I need to make sure to do this for all BSONObjs in all my request structs. It'd be great if TypedCommand instead parsed the incoming BSONObjs in owned manner.



 Comments   
Comment by Sandeep Dhoot [ 20/Apr/23 ]

Cool, that does sound like a big change. Please feel free to close the issue. Thanks for looking into this!

Comment by Sandeep Dhoot [ 20/Apr/23 ]

Sorry, I didn't fully get that. So if `request.body` is owned, any BSONObjs in the request body are also owned by this top level object and not individually owned at every level?

Is there any other quick fix e.g. can TypedCommand let the derived class decide how to parse the request - owned or unowned?

Comment by Billy Donahue [ 19/Apr/23 ]

The request body isOwned. It owns its own SharedBuffer.
I didn't realize this and thought the enclosing Message owned the SharedBuffer.

But the elements within the request are not self-owned. They're owned by that request body. They're all deferring all ownership concerns to the request body, which handles ownership so they don't have to.

If the request contains an array of 100 objects, we are avoiding having to refcount the request's whole body 100 times.

Comment by Sandeep Dhoot [ 19/Apr/23 ]

Thanks billy.donahue@mongodb.com for the context, this makes sense.

If this change could make things slower in the general case, then yeah, we don't need to make the change. I assumed that the `getOwned()` call in this case should be a no-op because probably the opMsgRequest that TypedCommand uses is already an owned BSONObj. I assumed so because there is this `isOwned` assert here. I am not able to tell why this OpMsgRequest instance is owned. If you know why, please share, I can then probably use the same trick in my code.

Comment by Billy Donahue [ 19/Apr/23 ]

If the TypedCommand parsing generated a structure that participated in ownership, we'd have slower parsing. The system we have today is optimized for the common case that the payload of a command is not retained by the server after the command ends.

The call to getOwned copies the BSONObj out of the enclosing Message. It is expensive and we can't do it by default because most commands won't benefit from owning the payload object. They only observe it.

It sounds like your command has the uncommon requirement of retaining the object after the command is destroyed. I think that it's reasonable for your command to explicitly declare this. I don't think such declarations should appear in IDL yaml files, as it's purely a detail of the C++ implementation of the command, rather than something intrinsic to the payload type.

I sympathize with the statement that this can be fragile. I think it's understood that BSONObj ownership is commonly considered too hard to reason about.

I could imagine a useful utility function to detach an IDL-generated struct from its source Message entirely via a deep copy. It could iterate through all BSONObj data members and replace them with getOwned() versions of themselves. But this wouldn't be super easy to write and might be too coarse anyway if the command only needs to retain some of the objects. This ideas is starting to get at a possibly generalized reflection API for IDL structs which would unlock other possibilities.

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