-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CXX-2705: Throw an exception upon unsuccessful pool creation #987
Conversation
src/mongocxx/pool.hpp
Outdated
@@ -109,6 +109,9 @@ class MONGOCXX_API pool { | |||
/// | |||
stdx::optional<entry> try_acquire(); | |||
|
|||
// Attempts to create a new client pool using the uri. Throws an exception upon error. | |||
void* construct_client_pool(const uri& uri); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unable to get mongoc_client_pool_t
to be the return type of this function.
/home/ubuntu/code/mongo-cxx-driver/src/mongocxx/../mongocxx/pool.hpp:112:5: error: 'mongoc_client_pool_t' does not name a type
112 | mongoc_client_pool_t* initialize_impl(const uri& uri);
It might be something small that I forgot to include, but that's the reason why this function returns a void *
and then casts the return value in the initializer list above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unable to get
mongoc_client_pool_t
to be the return type of this function.
The C driver API is explicitly hidden in public C++ driver headers. The C driver is intended to be an implementation detail in the driver. The private .hh
headers and .cpp
may use the C driver API.
Rather than make a private method on pool.hpp
, I suggest making a function in pool.cpp
:
static mongoc_client_pool_t* construct_client_pool(mongoc_uri_t* uri) {
bson_error_t error;
auto pool = libmongoc::client_pool_new_with_error(uri, &error);
if (!pool) {
// If constructing a client pool failed, throw an exception from the bson_error_t.
throw_exception<operation_exception>(error);
}
return pool;
}
src/mongocxx/pool.cpp
Outdated
@@ -132,5 +132,16 @@ stdx::optional<pool::entry> pool::try_acquire() { | |||
entry::unique_client(new client(cli), [this](client* client) { _release(client); })); | |||
} | |||
|
|||
void* pool::construct_client_pool(const uri& uri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
members of a class must be set in the initializer list of the constructor. Because we want to be able to call a function, check the response, and possibly throw an exception before setting _impl
(a const
member), this all must be done in a separate function to ensure that we can still set _impl
in the initializer list upon successful creation of a client pool.
src/mongocxx/pool.cpp
Outdated
static mongoc_client_pool_t* construct_client_pool(mongoc_uri_t* uri) { | ||
bson_error_t error; | ||
auto pool = libmongoc::client_pool_new_with_error(uri, &error); | ||
if (error.code) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm checking by error code here because the interpose
function for client_pool_new_with_error
in the a pool is created with the correct MongoDB URI
test returns nullptr
to the pool. Because of this, writing the if statement as if (!pool)
won't work. I'm unsure on if returning an empty mongoc_client_pool_t *
from the interpose is the correct solution here but I understand why checking error.code
isn't intuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking if (error.code)
may work, but I expect this may require bson_error_t error = {0};
to ensure error
is initialized when no error occurs. mongoc_client_pool_new_with_error
may not zero error
if no error occurs.
I suggest checking if (!pool)
and updating the test mocks to not use interpose
to replace the C driver function call. visit
can be used to call a function in addition to the C driver function.
Here is a suggested commit with a description: be69c67
Summary
This PR will ensure that a helpful exception is thrown to the user when a client pool cannot be constructed with the given URI.
Background
Prior to this PR, when the user provided an incorrect URI or the driver was unable to establish a connection to the server, an unhelpful error would display and the program would crash. This is because when
libmongoc::client_pool_new
is called with a URI that is unable to connect to a server, the function will returnnull
. This value would then be made into a unique pointer, be assigned to_impl
, and eventually would get caught down the line by any function that asserts thatpool != nullptr
.This is the reason for the unhelpful messaging that existed before, the empty pool object was not caught at the source.
What's New
construct_client_pool
, which will attempt to create amongoc_client_pool_t
usingclient_pool_new_with_error
and throw an exception upon errorpool
initializer list to utilize this new functionMOCK_POOL
changes to ensure that existing tests can pass now that we are usingclient_pool_new_with_error
instead ofclient_pool_new