[CXX-2043] Issue with initializing a mongocxx::pool::entry Created: 09/Jun/20  Updated: 01/Feb/23  Resolved: 23/Aug/20

Status: Closed
Project: C++ Driver
Component/s: API
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Puya Daravi Assignee: Unassigned
Resolution: Won't Fix Votes: 0
Labels: post-5.0
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Epic Link: CXX usability improvements
Quarter: FY24Q3

 Description   

Hello. `mongocxx::pool::entry` is designed to mimic `unique_ptr`. However it is created by a factory and therefor cannot be default constructed with a `nullptr` like how you would normally do with a `unique_ptr`.

This creates an awkward situation where you have to wrap the `entry` inside another unique_ptr to be able to separate declaration and definition (i.e. when using it as a class member). This means that you would need to have code that looks like this
`(**entry).list_databases():`

or
`(*entry)->list_databases():`

Adding an extra level of indirection as well as being hard to read.

I think the entry either should not mimic a `unique_ptr`, or if it does, then like a `unique_ptr` you should be able to default construct an invalid entry from a `nullptr` and move construct later on from what the pool factory gives you.



 Comments   
Comment by Puya Daravi [ 18/Jun/21 ]

Hi kevin.albertson,

I hope it is OK to ask a follow up after this long. Would you then say getting an entry from the pool (if `minPoolSize` is set to 0 and entry has already been created) is a very cheap operation? (compared to `collection->insert()` for example) I am keeping the entries in a `Collection` wrapper class in my design since I only expect to have a few concurrent clients. But if acquiring / releasing entries (that have already been created) are meant to be very cheap then I might go with your suggestion of not keeping the `entry` alive on the heap.

Also. Doesn't your suggestion to not keep a client go against the point made in the documentation here?
"In most programs, clients will be long lived - so it is less of a hassle (and more performant) to make one per-thread."

Comment by Kevin Albertson [ 23/Aug/20 ]

Hi puya@motionmetrics.com,

Thank you again for reporting, and for your patience. We've discussed internally and have decided not to implement a default constructor for mongocxx::pool::entry.

The expected use of a mongocxx::pool is for each thread to have a reference to the pool, and to only obtain a client pool entry for a short period of time when an entry is needed. Limiting the scope of entries permits more client reuse between threads.

We think making mongocxx::pool::entry default constructible could encourage users to keep a reference to pool entries longer than they should. In the less common case where you really need to keep a nullable pool entry, you can do so with an optional.

Comment by Kevin Albertson [ 22/Jul/20 ]

Hi puya@motionmetrics.com, thank you for the report! And apologies for the delayed response.

This creates an awkward situation where you have to wrap the `entry` inside another unique_ptr to be able to separate declaration and definition (i.e. when using it as a class member)

Alternatively, you could wrap this in an optional if you want a nullable pool::entry. For example:

bsoncxx::stdx::optional<mongocxx::pool::entry> entry_opt;
// ...
entry_opt = pool.acquire();

Adding an extra level of indirection as well as being hard to read.

Yes, wrapping a pool::entry in an optional or unique_ptr does mean that obtaining the underlying client requires another dereference. Though, you could store the result of the dereference in a mongocxx::client reference to limit the places that require double dereferencing.

mongocxx::client& client = *(*entry_opt);

Looking at some of the history, mongocxx::pool::entry was originally an alias for a unique_ptr. But that was changed in CXX-1184. The motivation was to prohibit use of temporary mongocxx::client objects obtained from pool. That changed mongocxx::pool::entry to be a unique_ptr-like class to enforce those prohibitions. But, no default constructor was added (only a private constructor to create a pool entry with a client).

mongocxx::pool::entry can already be marked invalid by assigning std::nullptr.

mongocxx::pool::entry entry = pool.acquire();
// Clear an entry.
entry = nullptr;

And it overrides a bool operator, so it can be checked for validity:

if (entry) {
   // do something with entry
} else {
   // entry is unset
}

So I think I agree. Adding a default constructor to mongocxx::pool::entry seems reasonable, and in my opinion agrees with the intent of the class.

One potential wart of adding the default constructor, is that there'd be two concepts within the driver to represent an invalid pool entry. mongocxx::pool::try_acquire returns a stdx::optional<mongocxx::pool::entry>.
 

Generated at Wed Feb 07 22:04:42 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.