v1: client (CXX-3237, CXX-3238)#1559
Conversation
| /// @param mongodb_uri | ||
| /// A MongoDB URI representing the connection parameters | ||
| /// @param options | ||
| /// Additional options that cannot be specified via the mongodb_uri |
There was a problem hiding this comment.
| /// Additional options that cannot be specified via the mongodb_uri | |
| /// Additional client options |
Suggest removing "cannot be specified". Some TLS options can be specified in either the URI (e.g. tlsCertificateKeyFile) or options (e.g. pem_file).
| /// Creates a new client connection to MongoDB. | ||
| /// | ||
| MONGOCXX_ABI_EXPORT_CDECL() ~client(); | ||
|
|
||
| client(client const&) = delete; | ||
| client& operator=(client const&) = delete; | ||
| /// @param mongodb_uri | ||
| /// A MongoDB URI representing the connection parameters |
There was a problem hiding this comment.
| /// Creates a new client connection to MongoDB. | |
| /// | |
| MONGOCXX_ABI_EXPORT_CDECL() ~client(); | |
| client(client const&) = delete; | |
| client& operator=(client const&) = delete; | |
| /// @param mongodb_uri | |
| /// A MongoDB URI representing the connection parameters | |
| /// Creates a new client to MongoDB. | |
| /// | |
| /// @param mongodb_uri | |
| /// A MongoDB URI |
Suggest removing "connection" to avoid confusing with a network connection (of which a client has many).
| struct to_mongoc_type { | ||
| std::list<bsoncxx::v_noabi::string::view_or_value> string_owner; | ||
|
|
||
| mongoc_ssl_opt_t opt; |
There was a problem hiding this comment.
mongoc_ssl_opt_t is conditionally defined in libmongoc. This may need to restore a #if MONGOCXX_SSL_IS_ENABLED() check to successfully build against a C driver built without SSL support.
There was a problem hiding this comment.
Good catch.
Further investigation also revealed cases where the v1::client test cases were not prepared for code paths triggered by an ENABLE_SSL=OFF configuration. Updated accordingly.
The internal call to v1::uri::tls() has been moved out of the ifdef blocks for consistent invocation count by mocked test cases.
src/mongocxx/test/v1/client.cpp
Outdated
| mocks.client_destroy->interpose([&](mongoc_client_t* ptr) -> void { | ||
| if (ptr) { | ||
| if (ptr != kv_client_id) { | ||
| FAIL_CHECK("unspected mongoc_client_t"); |
There was a problem hiding this comment.
| FAIL_CHECK("unspected mongoc_client_t"); | |
| FAIL_CHECK("unexpected mongoc_client_t"); |
There was a problem hiding this comment.
Excuse the delay. GCC on RHEL 8 ARM64 does not seem to like the client_mocks_type pattern. For reasons unclear, it specifically dislikes the "auto_encryption_opts" test case and segfaults during destruction. Stacktrace points to std::lock_guard<std::mutex> lock(_active_instances_lock); in mock<T>::destroy_active_instance(); enabling ASAN suppresses the issue (!?); UBSAN reports out-of-alignment access of a parent mock object from _parent->destroy_active_instance() in the mock<T>::instance dtor (???); Clang does not exhibit this behavior. Therefore, I suspect this may be a GCC compiler codegen bug specific to ARM64(?), but could not find any relevant bug reports. Dynamically allocation appears to be a viable workaround, so the "auto_encryption_opts" test case uses std::unique_ptr<client_mocks_type> mocks_owner; instead of the usual client_mocks_type mocks;.
| struct to_mongoc_type { | ||
| std::list<bsoncxx::v_noabi::string::view_or_value> string_owner; | ||
|
|
||
| mongoc_ssl_opt_t opt; |
There was a problem hiding this comment.
Good catch.
Further investigation also revealed cases where the v1::client test cases were not prepared for code paths triggered by an ENABLE_SSL=OFF configuration. Updated accordingly.
The internal call to v1::uri::tls() has been moved out of the ifdef blocks for consistent invocation count by mocked test cases.
Resolves CXX-3237 and CXX-3238 for the
v1::clientcomponent.Some fixes and improvements to CXX-3236 include:
v1::client::client()'soptionsparameter fromT const&->Tto permit moving the APM callbacks (unlike v_noabi which requires copies due toT const&).v1::clientinline within the class definition.v1::client::watch()overloads so thev1::change_stream::optionsparameter is consistentlyT const&(a few wereTdespite no ownership transfers).Due to acyclic-but-mutual-dependencies between the
client,pool,client_session, anddatabasecomponents, some functions are left unimplemented and marked with// TODO: <component> (CXX-3237)comments. These comments will be addressed by upcoming associated component PRs. However, the refactored implementation ofv_noabi::clientmirrors the upcoming equivalent v1 implementations forv1::clientand may be used as a preview/reference of the upcoming v1 implementation. Note that a stub forv1::client_session::options::~options()is required by MSVC to satisfy the definition(s) ofv1::client::watch()(similar to the out-of-line virtual dtor definition requirement for CXX-3236; this does not affect other compilers).For consistency with other refactored v_noabi components, the conversion helpers for
v_noabi::options::*required byv_noabi::clientare relocated and renamed to*::internal::to_mongoc(). There is some code duplication between the v_noabi and v1 implementation in order to avoid making intermediate copies that would be required by v_noabi <-> v1 conversions due toT const¶meter types. We can consider adding rvalue-ref overloads to allow opting-into a more efficient implementation (i.e. ownership transfer of APM callbacks), but I've opted not to do this for now.Note the updated v_noabi tests for
v_noabi::clientandv_noabi::poolare due to the relocation of thetls_optionsdata member out ofv_noabi::client::impl. This data member appears to have been motivated by the (old) mock tests which assumed the lifetime ofv_noabi::string::view_or_value::terminated()-owning strings for TLS options will be extended by the client object. This behavior is AFAIK not observable by the public API and is therefore removed in favor of a temporary , though the view-or-value behavior is preserve in the return value ofv_noabi::options::tls::internal::to_mongoc().For consistency with
scoped_bson::operator+=(), rather than ignoring return values, calls toBSON_APPEND_*()instead throw undocumentedstd::logic_errorexceptions to indicate contract violations. This only affects the "comment" field (noBCON_VALUE()forbson_value_t) and the "startAtOperationTime" field (BCON_TIMESTAMP()has incorrect parameter types).v_noabi::options::clientinherits the v_noabi <-> v1 key vault client (and pool) conversion problem described in #1551, due to the.auto_encryption_opts()field exposing the key vault client field. Therefore, it is givenfrom_v1()/to_v1()overloads in a manner similar tov_noabi::options::auto_encryption.However,
v_noabi::clientdoes not inherit this problem: the associated key vault client being stored by the mongoc library asmongoc_client_t*, not by the mongocxx library asv1::client*. Because the stored key vault client is not exposed by thev_noabi::client(orv1::client) public API (e.g. there is no.auto_encryption_opts().key_vault_client() -> v1::client*), the associated key vault client only needs to outlive thev_noabi::client(as usual) and use themongoc_client_tmanaged by the underlyingmongoc_client_tobject. This is similar to howv_noabi::index_viewandv_noabi::search_index_viewstoremongoc_client_t*rather thanv1::client*; this pattern has been extended tov_noabi::databaseandv_noabi::collectionfor consistency.