-
Notifications
You must be signed in to change notification settings - Fork 547
CXX-3324 remove all use of instance::current() #1438
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
kevinAlbs
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.
Changes look good. I vote for leaving instance::current un-deprecated.
|
|
||
| ### Deprecated | ||
|
|
||
| - `mongocxx::v_noabi::instance::current()` is "for internal use only". The `instance` constructor(s) should be used instead. |
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 see usage of instance::current() in GitHub code search. I suspect it might be useful to conditionally initialize the C++ driver on first use (for example in an extension).
Is there much harm in keeping instance::current()? If there are multiple users, I would rather leave it un-deprecated.
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.
Is there much harm in keeping
instance::current()?
The motivation is to discourage usage patterns which require global state (e.g. see CDRIVER-1547; I don't think we have a C++ Driver equivalent ticket yet). The behavior of instance::current() is fundamentally global (the function static local variable is an implementation detail). By deprecating and removing this function, users will be forced to explicitly decide their instance object allocation strategy; they have to create an instance object somewhere. This will ultimately provide us with better opportunities to remove global state (by moving state into the instance object) while preserving global mongoc initialization/cleanup behavior internally rather than via a dedicated API (that is, we reduce the scope of the current global atomic variable to handling one-time mongoc init/cleanup only, per CXX-1029, until mongoc also hopefully/eventually removes its global state requirements).
The code search results appear to support the case that most users can easily substitute mongocxx::instance::current(); with static mongocxx::instance current;, as all the projects appear to have only one substantial call to current() within their the codebase (that is not test or example 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.
most users can easily substitute
mongocxx::instance::current();withstatic mongocxx::instance current;
Good point. Suggest expanding the changelog entry to help users migrate. Example:
If an application only calls
mongocxx::instance::current();once, it can likely be replaced withstatic mongocxx::instance current;:// Before: void handler () { mongocxx::instance::current(); // ... code using mongocxx ... } // After: void handler () { static mongocxx::instance current; // ... code using mongocxx ... }
|
|
||
| ### Deprecated | ||
|
|
||
| - `mongocxx::v_noabi::instance::current()` is "for internal use only". The `instance` constructor(s) should be used instead. |
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.
most users can easily substitute
mongocxx::instance::current();withstatic mongocxx::instance current;
Good point. Suggest expanding the changelog entry to help users migrate. Example:
If an application only calls
mongocxx::instance::current();once, it can likely be replaced withstatic mongocxx::instance current;:// Before: void handler () { mongocxx::instance::current(); // ... code using mongocxx ... } // After: void handler () { static mongocxx::instance current; // ... code using mongocxx ... }
Resolves CXX-3324 by removing all use of
mongocxx::v_noabi::instance::current()except insrc/mongocxx/test/v_noabi/instance.cpp(to preserve some test coverage of this API until its removal).The
current()function was added tomongocxx::v_noabi::instancea long time ago by #441 (Jan 2016) to address ASAN and Valgrind memory leak errors. #513 (Sep 2016) later added the clarification thatcurrent()is "intended primarily for test authors". At this time, Catch was a header-only library, specifically the "CATCH v1.0 build 53 (master branch)" header generated on August 2014. This version predates the first ever stable release by nearly a year. The header was updated to v1.9.4 on June 2017, to v2.2.1 on April 2018, then to v3.7.0 on September 2024. That is to say, the codebase has changed quite a lot sincecurrent()was added.#1202 added a
catch.cppfile providing a user-definedmain()function as part of__cdeclchecks on Windows (to avoid the Catch2-suppliedmain()from incorrectly inheriting the non-cdecl default calling convention). Thismain()function is the perfect place to create themongocxx::v_noabi::instanceobject for subsequent use by the test suite. The only exceptions are thetest_instanceandtest_loggingexecutables which need to create their own instance object to testmongocxx::v_noabi::instanceitself. All other cases of callingcurrent()function is made obsolete, and therefore, removed by this PR.Note
This is similar in spirit to the API examples runner creating a
mongocxx::instanceobject prior to executing non-forking components.It is premature to remove
current()entirely, given it's been present in both the publically-documented API and ABI for a very long time (there are almost certainly users depending on it despite the "intended primarily for test authors" note). However, a deprecation note has been added to CHANGELOG declaring its new "for internal use only" documentation. AFAICT there is no reason to continue supporting thecurrent()function as part of the public API aside from its "initialize if not already initialized" behavior (which was really only ever meant to be used for testing purposes), as there is no reason to obtain the "current" instance object for any subsequent purpose (theinstanceclass has no meaningful API beyond its constructors + SMFs).Note
CXX-1029 proposes adding API to the
instanceclass to support inspecting and modifying library logging behavior in addition to mongocxx (and mongoc) library initialization and cleanup. We can consider re-introducing acurrent()function alongside such logging-related API, likely as an extension to the v1 API rather than to the v_noabi API.After this PR, the only remaining use of the
current()function is ininstance.cppto preserve test coverage of the function until it is ultimately removed in an upcoming API major version release. (The tests were also fixed to avoid assuming test-case-declaration execution order.) TheMONGOCXX_TEST_DISABLE_MAIN_INSTANCEpreprocessor macro is defined when building thetest_instanceandtest_loggingtest executables to disable the creation of an instance object inmain(); all other mongocxx test executables create an instance object inmain()prior to invoking the Catch2 test runner.