[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 or 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? | ||||||||||||
| Comment by Kevin Albertson [ 23/Aug/20 ] | ||||||||||||
|
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.
Alternatively, you could wrap this in an optional if you want a nullable pool::entry. For example:
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.
Looking at some of the history, mongocxx::pool::entry was originally an alias for a unique_ptr. But that was changed in mongocxx::pool::entry can already be marked invalid by assigning std::nullptr.
And it overrides a bool operator, so it can be checked for validity:
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>. |