-
Notifications
You must be signed in to change notification settings - Fork 548
v1: apm and events (CXX-3237, CXX-3238) #1524
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
Conversation
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.
LGTM with some minor fix-ups and a question to confirm my understanding.
connorsmacd
left a comment
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.
LGTM with one weak suggestion.
| std::size_t n = {}; | ||
| auto const sds = libmongoc::topology_description_get_servers(to_mongoc(_impl), &n); | ||
|
|
||
| // Transfer ownership of each element, but not the parent array. | ||
| ret.reserve(n); | ||
| std::transform(sds, sds + n, std::back_inserter(ret), [](mongoc_server_description_t* sd) { | ||
| return v1::events::server_description::internal::make(sd); | ||
| }); | ||
| bson_free(sds); |
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.
| std::size_t n = {}; | |
| auto const sds = libmongoc::topology_description_get_servers(to_mongoc(_impl), &n); | |
| // Transfer ownership of each element, but not the parent array. | |
| ret.reserve(n); | |
| std::transform(sds, sds + n, std::back_inserter(ret), [](mongoc_server_description_t* sd) { | |
| return v1::events::server_description::internal::make(sd); | |
| }); | |
| bson_free(sds); | |
| auto const sds_deleter = [](mongoc_server_description_t** sds) { bson_free(sds); }; | |
| using sds_unique_ptr_t = std::unique_ptr<mongoc_server_description_t**, decltype(sds_deleter)>; | |
| std::size_t n = {}; | |
| auto const sds = sds_unique_ptr_t(libmongoc::topology_description_get_servers(to_mongoc(_impl), &n), sds_deleter); | |
| // Transfer ownership of each element, but not the parent array. | |
| ret.reserve(n); | |
| std::transform(sds.get(), sds.get() + n, std::back_inserter(ret), [](mongoc_server_description_t* sd) { | |
| return v1::events::server_description::internal::make(sd); | |
| }); |
Suggest making this exception-safe since reserve can throw. However, that exception is highly unlikely and would probably mean the program is running OOM, so I don't feel strongly about this.
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.
Support for OOM is inconsistently-but-generally-not-supported as far as I am aware. Nevertheless, .reserve() can also potentially throw std::length_error (although I expect this to be equally unlikely), so for the sake of exception correctness, opted to guard sds as suggested. However, given the scope of potentially-throwing code is limited to just the .reserve() call, used a try-catch block instead of std::unique_ptr (also note the different deleter required before vs. after the call to std::transform()). As far as I can tell, no other instance of a call to .reserve() currently contains an unguarded owning resource in its immediate scope.
Resolves CXX-3237 and CXX-3238 for the
v1::apmandv1::events::*components.Due to the
T const&accessor problem, thev_noabi::options::apmclass only implements support forv_noabi <-> v1conversion and ABI footprint reduction. However, because thev_noabi::events::*classes are (mostly) non-owning read-only views of underlyingmongoc_apm_*objects owned by mongoc, eachv_noabi::events::*class is implemented in terms of the equivalentv1::events::*class (except forv1::events::server_description).Due to the
v1::events::*classes being uniquely non-owning view-like API for mongoc-owned event objects, their tests are implemented together intest/v1/events.cppto consolidate and reuse the mock patterns necessary to construct and test the event class API. Note that, whenever able, these mocked tests compare the identity (address) of values passed to and returned from mongoc functions rather than their individual values (not relevant to the behavior being tested), hence the plethora of explicit static and reinterpret casts.Additional notes:
v1::apm::server_heartbeat_()accessors to match the renamed event classes + some missing export macros inv1::events::server_description. These issues are fixed by this PR.v_noabi::events::*class' publicvoid const*ctors are now explicitly documented as "internal use only" rather than removed. To avoid potential confusion due to appearing to be part of the class public API, an explicitMONGOCXX_ABI_NO_EXPORTmacro is also applied (successor to the oldMONGOCXX_PRIVATEmacro).mongoc_topology_description_*()has not required non-const argument since CDRIVER-4114 (see also CDRIVER-2809), so thetopology_description-related implementation no longer needs to useconst_cast. Thevoid*ctor is also updated tovoid const*accordingly (not an API or ABI breaking change).v_noabi::events::server_descriptionmay also exhibit the "transitive view" problem in contexts where ownership may escape the callback function (e.g. return value oftopology_description::servers()), the conversion fromv1::events::server_descriptionrequires (via an "Important" admonition in the docs) the v1 object outlives the v_noabi object. For consistency with the rest of v1 API, "maybe-owning" behavior is avoided (i.e.view_or_value,bson_t, etc.).apmclass is straightforward due tostd::function<T>'s converting constructors (av1::events::*argument can implicitly convert to a callback'sv_noabi::events::*parameter). The opposite is not true due to v_noabi -> v1 for event classes being an explicit conversion (av_noabi::events::*argument cannot implicitly convert to a callback'sv1::events::*parameter). Therefore, an intermediate function that explicitly converts the argument from v_noabi -> v1 is required. This would be trivial with lambda expressions; however, the required init-capture form to permit move-capturing the callback function via[x = std::move(init)]is only available with C++14 and newer. Therefore, theinvoke_as_v1<Fn>class template is used instead as a workaround until the minimum C++ standard can be bumped to C++14 or newer. If preferable, we can avoid this boilerplate by copy-capturing the callback instead using[fn]; I've opted forstd::move_only_function<T>-like behavior for now.__cdeclcannot be specified for lambda expressions for MSVC. This conflicts with compiling test code with__vectorcall(see CXX-3092) when comparing the identity of registered APM callbacks intest/v1/apm.cpp. Thankfully, MSVC does support implicit conversion to function pointers with the correct calling convention; this is implemented via thefn_typetype aliases. This should be unnecessary once CXX-3310 is resolved (dropping support for default calling conventions that are not__cdecl).