CXX-3320 migrate instance and logger to mongocxx::v1 #1469

eramongodb · 2025-09-26T19:21:45Z

Related to CXX-3320. Due to the unique nature of mongocxx::v_noabi::instance as a global singleton-like object, this class is migrated in full from v_noabi to v1 as a single, standalone PR. Related types v_noabi::logger and v_noabi::exception are also minimally migrated as part of this PR to support the instance class API.

First, concerning v_noabi::logger: due to having no observable difference in behavior in v1, v_noabi::logger is merely a redeclaration of v1::logger in the v_noabi namespace! 🎉 This also avoids complications with trying to support compatibility between both a v_noabi::logger and v1::logger class given their polymorphic nature.

The v_noabi::instance class is not so fortunate. Its migration is complicated primary by the realization that there are two unresolvable deficiencies with the v_noabi::instance API:

it does not support using the mongoc default log handler, and
the custom log handler (if any) is registered after mongoc_init() has already been invoked.

As mentioned by mongoc docs:

Note that in the example above mongoc_log_set_handler() is called before mongoc_init(). Otherwise, some log traces could not be processed by the log handler.

That is, the initialization behavior is currently defined as follows (pseudocode):

void v_noabi::init(mongoc_log_func_t handler) {
    mongoc_init(); // May emit log messages!

    if (handler) {
        mongoc_log_set_handler(handler); // Already initialized!
    } else {
        mongoc_log_set_handler(nullptr); // Disable log messages!
    }

    // ... handshake ...
}

The v1::instance object proposes fixing these issues as follows:

Support a v1::default_handler tag type by which the user can request using the mongoc default log handler.
Register or disable the custom log handler before calling mongoc_init() so that all log messages are handled properly.

That is, the initialization behavior is changed to the following:

void v1::init(mongoc_log_func_t handler) {
    v1::init::impl(handler, true); // Set a custom log handler.
}

void v1::init(default_handler /* tag */) {
    v1::init::impl(nullptr, false); // Use default log handler.
}

void v1::init::impl(mongoc_log_func_t handler, bool set_custom_handler) {
    // Support using the default log handler.
    if (set_custom_handler) {
        if (handler) {
            mongoc_log_set_handler(handler); // Already initialized!
        } else {
            mongoc_log_set_handler(nullptr); // Disable log messages!
        }
    }

    mongoc_init(); // All log messages are captured.

    // ... handshake ...
}

For backward compatibility, v_noabi::instance's custom log handler constructor preserves the "register handler after mongoc_init()" behavior. If we do not consider this behavior to be worth preserving (how important are log messages which may be emitted by mongoc_init()?), we can simplify the implementation of v_noabi::instance's initialization. (Related: CXX-1029) Update: per feedback, simplified the v_noabi implementation to also register the custom unstructured log handler before mongoc initialization.

The v_noabi::instance still supports the (deprecated) ::instance() helper function for backward compatibility. The implementation of ::instance() remains specific to v_noabi: the implementation v1::instance is entirely unaware of ::instance() support. In place of the current_instance "sentinel" value, v1::instance instead utilizes a simple std::atomic_int counter to track and report exceptions given multiple instance objects.

Much of the rest of this PR involves refactors to the test suite for the instance and logger classes. The test_logging executable is removed in favor of grouping all instance object testing to test_instance. All other test executables can (in theory) be grouped into a single test executable without concern for "multiple instance object" exceptions.

The fork-based pattern to test instance objects used by the examples API runner (#1216) is repurposed here for compatibility with the Catch2 test suite as mongocxx::test::subprocess(op). This helper function accepts a single op: void() invocable that is executed within a forked subprocess. The result of the subprocess, including termination, is translated into well-behaved Catch test assertions. The Catch test suite reporter is reused by the subprocess to support capturing values and evaluating assertions. However, using SECTION() is not supported due to non-trivial control flow, either within op or in the parent TEST_CASE().

This enabled writing test cases asserting proper log message handling behavior, including default log message handler behavior, by capturing log output emitted by the subprocess (using POSIX pipe API in the capture_stderr class). Note this is only supported on non-Windows platforms. Unfortunately, the use of std::exit() and std::terminate() in the subprocess confuses valgrind, which (understandably) reports memory leaks due to the abnormal process termination method. Therefore, leak detection in subprocesses is disabled for the test_instance executable using --trace-children=no.

eramongodb · 2025-09-26T19:23:41Z

src/mongocxx/include/mongocxx/v1/instance.hpp

+    instance& operator=(instance const&) = delete;
+
+    ///
+    /// Initialize the mongoc library with unstructured log messages disabled.


Updated documentation to clarify that this all applies to "unstructured log handling" to leave open the design space for "structured log handling" support by the C++ Driver.

Documentation also increased references to specific mongoc API to clarify behavior + defer much of the behavioral documentation to mongoc.

eramongodb · 2025-09-26T19:28:50Z

src/mongocxx/include/mongocxx/v1/instance.hpp

+    ///
+    /// This class is not moveable.
+    ///
+    instance(instance&&) = delete;
+
+    ///
+    /// This class is not moveable.
+    ///
+    instance& operator=(instance&&) = delete;
+
+    ///
+    /// This class is not copyable.
+    ///
+    instance(instance const&) = delete;
+
+    ///
+    /// This class is not copyable.
+    ///
+    instance& operator=(instance const&) = delete;


Relative breaking change: instance is made immovable as well as uncopyable. Allowing instance to be moved-from does not seem to be well-motivated.

eramongodb · 2025-09-26T19:30:53Z

src/mongocxx/lib/mongocxx/v1/instance.cpp

+    mongoc_log_level_t log_level,
+    char const* domain,
+    char const* message,
+    void* user_data) noexcept {


Small relative change: added noexcept for consistency with other C API callback functions.

eramongodb · 2025-09-26T19:33:37Z

src/mongocxx/lib/mongocxx/v1/instance.cpp

+                "mongocxx",
+                MONGOCXX_VERSION_STRING,
+                "CXX=" MONGOCXX_COMPILER_ID " " MONGOCXX_COMPILER_VERSION " stdcxx=" STDCXX " / ");


Minor optimization: avoid allocations, given all fields are literals, by concatenating everything into a single string literal.

src/mongocxx/test/subprocess.cpp

kevinAlbs

LGTM with minor comments. I like the new overload to simplify enabling default logging.

how important are log messages which may be emitted by mongoc_init()?

I see two possible logs:

1 Failure to initialize OpenSSL.
2 Failure to get OS version for handshake.

I expect neither is likely. (2) might be improved by making handshake non-global (CDRIVER-4142) (errors could be returned on the first client operation).

For backward compatibility, v_noabi::instance's custom log handler constructor preserves the "register handler after mongoc_init()" behavior.

If it helps simplify: I expect having v_noabi register the log handler before mongoc_init (to match v1) is unlikely to harm consumers (some added unlikely-to-occur logs)

kevinAlbs · 2025-10-01T19:06:05Z

src/mongocxx/test/subprocess.hh

+// @returns The exit code of the subprocess (`*is_signal` is `false`) or the signal used to kill the subprocess
+// (`*is_signal` is `true`) .


Suggested change

// @returns The exit code of the subprocess (`*is_signal` is `false`) or the signal used to kill the subprocess

// (`*is_signal` is `true`) .

// @returns The exit code of the subprocess (`*is_signal_ptr` is `false`) or the signal used to kill the subprocess

// (`*is_signal_ptr` is `true`) .

kevinAlbs · 2025-10-01T19:11:48Z

src/mongocxx/test/subprocess.cpp

+    CHECK_SUBPROCESS([] {
+        // Try to silence noisy Catch2 output.
+        (void)::close(1); // stdout
+        (void)::close(2); // stderr
+
+        SKIP("subprocess");
+    });


Suggested change

CHECK_SUBPROCESS([] {

// Try to silence noisy Catch2 output.

(void)::close(1); // stdout

(void)::close(2); // stderr

SKIP("subprocess");

});

CHECK_SUBPROCESS([] {

// Try to silence noisy Catch2 output.

(void)::close(1); // stdout

(void)::close(2); // stderr

SKIP("subprocess");

}, &is_signal);

kevinAlbs · 2025-10-01T19:40:12Z

src/mongocxx/test/v1/logger.cpp

+
+} // namespace
+
+// mongocxx::v1::logger must not unnecessarily mpose special requirements on derived classes.


Suggested change

// mongocxx::v1::logger must not unnecessarily mpose special requirements on derived classes.

// mongocxx::v1::logger must not unnecessarily impose special requirements on derived classes.

kevinAlbs · 2025-10-01T20:02:29Z

src/mongocxx/include/mongocxx/v1/instance.hpp

+    ///
+    /// Cleanup the mongocxx (and mongoc) library.
+    ///
+    /// Calls [`mongoc_init()`](https://mongoc.org/libmongoc/current/mongoc_cleanup.html).


Suggested change

/// Calls [`mongoc_init()`](https://mongoc.org/libmongoc/current/mongoc_cleanup.html).

/// Calls [`mongoc_cleanup()`](https://mongoc.org/libmongoc/current/mongoc_cleanup.html).

kevinAlbs · 2025-10-01T20:09:05Z

src/mongocxx/lib/mongocxx/v1/instance.cpp

+static_assert(!std::is_move_constructible<instance>::value, "bsoncxx::v1::instance must be non-moveable");
+static_assert(!std::is_copy_constructible<instance>::value, "bsoncxx::v1::instance must be non-copyable");


Suggested change

static_assert(!std::is_move_constructible<instance>::value, "bsoncxx::v1::instance must be non-moveable");

static_assert(!std::is_copy_constructible<instance>::value, "bsoncxx::v1::instance must be non-copyable");

static_assert(!std::is_move_constructible<instance>::value, "mongocxx::v1::instance must be non-moveable");

static_assert(!std::is_copy_constructible<instance>::value, "mongocxx::v1::instance must be non-copyable");

eramongodb

If it helps simplify: I expect having v_noabi register the log handler before mongoc_init (to match v1) is unlikely to harm consumers (some added unlikely-to-occur logs)

Simplified the v_noabi implementation as suggested.

eramongodb · 2025-10-02T14:23:21Z

src/mongocxx/lib/mongocxx/v_noabi/mongocxx/instance.cpp

-
-            // Inform the user that a custom log handler has been registered.
-            // Cannot use mocked `libmongoc::log()` due to varargs.
-            mongoc_log(MONGOC_LOG_LEVEL_INFO, "mongocxx", "libmongoc logging callback enabled");


Do we want to keep this informational log message?

I am slightly in favor of keeping for v_noabi. The API examples expect it, and it might avoid a surprise to consumers. I expect it is not much burden for us to keep it in v_noabi. But I do not think it is needed for v1.

vector-of-bool

LGTM with only a few minor comments.

vector-of-bool · 2025-10-21T17:33:01Z

src/mongocxx/include/mongocxx/v1/exception.hpp

 ///
-enum class source_errc {};
+enum class source_errc {
+    zero,       ///< Zero.


Should this be zero, or something like okay? Alternatively: set mongocxx = 1, give the enum a base type like int, and then have source_errc{} as the stand-in for zero, which is the non-error syntax for std::errc.

(I don't feel strongly about any of the above options, just something to consider.)

Should this be zero, or something like okay? Alternatively: set `mongocxx = 1 [...]

My reason for including the zero enumerator was twofold:

Avoid the need for explicit = 1 in the otherwise =-less list of enumerators (an inconsistency).

Force explicit handling of the zero case in switch cases via -Wswitch / -Wswitch-enum.

I opted for zero instead of okay to avoid implying anything beyond a description of the value itself, since "not zero" does not necessarily imply "not okay" or "not valid" given certain error code schemes (i.e. "HTTP status code 200").

give the enum a base type like int

The default underlying type for a scoped enumeration is int, so there is no need for explicit : int given enum class E.

vector-of-bool · 2025-10-21T17:34:46Z

src/mongocxx/lib/mongocxx/v1/exception.cpp

+    return instance.value();
+}
+
+exception::~exception() = default;


Is there a benefit to providing the defaulted destructor out-of-line? This inhibits inlining of the exception destructor without LTO enabled.

This is the key function for v1::exception in lieu of any other virtual functions:

[...] if the class has a key function (see below), the tables are emitted in the object for the translation unit containing the definition of the key function. This is unique if the key function is not inline. Otherwise, the tables are emitted in every object that references any of them.

and:

The key function is the first non-pure virtual function that is not inline at the point of class definition. [...] Note that if the key function is not inline in the class definition, but its later definition is inline, it will be emitted in every object containing the definition.

The absence of a key function triggers the -Wweak-vtables Clang warning.

On second thought, it may be preferable to use a private virtual .key_function() for the sole purpose of acting as the key function instead of the destructor, which has the additional benefit of avoiding Ro5 boilerplate:

class exception : public std::system_error { public: using std::system_error::system_error; private: MONGOCXX_ABI_NO_EXPORT virtual void key_function() const; };

Will propose this pattern for review in a followup PR.

vector-of-bool · 2025-10-21T17:37:27Z

src/mongocxx/include/mongocxx/v1/exception.hpp

+namespace std {
+
+template <>
+struct is_error_condition_enum<mongocxx::v1::source_errc> : true_type {};
+
+template <>
+struct is_error_condition_enum<mongocxx::v1::type_errc> : true_type {};
+
+} // namespace std


Minor tweak option: You can omit the outer namespace std and do:

template <> struct std::is_error_condition_enum<...>

with the std:: qualifier on the struct name. This isn't strictly better, but may reduce the amount of visual noise.

I initially attempted this pattern as well, but encountered compilation errors with GCC versions older than 7.1 (possibly related: CWG 275). However, we now require GCC 8 or newer as of #1415, so I think the suggestion is now viable:

mongo-cxx-driver/CMakeLists.txt

Lines 28 to 33 in cbb70da

# https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html

# https://gcc.gnu.org/projects/cxx-status.html

# https://gcc.gnu.org/releases.html

if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1")

message(FATAL_ERROR "GCC 8.1 or newer is required")

endif()

vector-of-bool · 2025-10-21T17:42:36Z

src/mongocxx/include/mongocxx/v1/logger.hpp

+    virtual void operator()(
+        log_level level,
+        bsoncxx::v1::stdx::string_view domain,
+        bsoncxx::v1::stdx::string_view message) noexcept = 0;


May be beyond the scope of this PR, but we may want to defer the creation of message based on whether level is enabled in the logger, otherwise the user pays for the string-formatting of every message for every level, even if they e.g. don't have trace enabled. (mongoc already does this, but only for the trace level.)

Also recommend doing the "private virtual" idiom so that the base class can have logic (i.e. conditional formatting) in operator(), and send the message on to a private virtual like do_write_log(level, domain, msg).

we may want to defer the creation of message based on whether level is enabled in the logger

I believe it would be more appropriate to implement this in mongoc (as being done for the trace level) rather than in mongocxx. The message parameter is merely a view of whatever mongoc_log() has already parsed and formatted.

recommend doing the "private virtual" idiom

This would will force incompatibility with the current v_noabi API, making the using v1::logger; alias within mongocxx::v_noabi unviable. This may be better suited as a followup extension to v1 API (i.e. part of CXX-2586).

I believe it would be more appropriate to implement this in mongoc (as being done for the trace level) rather than in mongocxx

May need to do both, as (IIUC) there is no mongocxx API that sends down a va_list to mongoc, and there is no mongoc API that accepts a va_list for logging.

Alternatively: I'm misremembering things and this is a non-issue 🙃

CXX-3320 migrate instance and logger to mongocxx::v1

efb65cb

eramongodb requested a review from vector-of-bool September 26, 2025 19:21

eramongodb self-assigned this Sep 26, 2025

eramongodb requested a review from a team as a code owner September 26, 2025 19:21

eramongodb requested a review from kevinAlbs September 26, 2025 19:21

eramongodb added 2 commits September 26, 2025 14:50

Remove redundant redeclarations

2892b35

Remove redundant instance ctor overload

0370746

eramongodb commented Sep 26, 2025

View reviewed changes

vector-of-bool reviewed Sep 29, 2025

View reviewed changes

src/mongocxx/test/subprocess.cpp Outdated Show resolved Hide resolved

eramongodb added 2 commits September 29, 2025 15:09

Use std::_Exit instead of std::exit in subprocess

4508456

Tweak rationale for disabling Valgrind leak checks for subprocesses

40218af

eramongodb requested a review from vector-of-bool September 30, 2025 14:19

kevinAlbs approved these changes Oct 1, 2025

View reviewed changes

eramongodb added 6 commits October 2, 2025 09:19

Merge remote-tracking branch 'upstream/master' into HEAD

3483624

Fix typos in comments and strings

75d5e9e

Fix missing &is_signal argument to subprocess

cc5339f

Simplify v_noabi implementation (capture pre-init logs)

ba058a4

Document post-move state for instance class

91b3d62

Document non-copyability for instance class

20b931c

eramongodb commented Oct 2, 2025

View reviewed changes

eramongodb added 5 commits October 2, 2025 10:15

Preserve v_noabi log handler enabled message

df7bf53

Correctly condition v_noabi info message

f24f3b6

Formatting

0be7784

Merge remote-tracking branch 'upstream/master' into cxx-abi-v1-instance

f2ecef1

Merge remote-tracking branch 'upstream/master' into cxx-abi-v1-instance

d2de21a

eramongodb mentioned this pull request Oct 9, 2025

CXX-3236 add mongocxx v1 declarations #1482

Open

15 tasks

Merge remote-tracking branch 'upstream/master' into cxx-abi-v1-instance

bf715d7

vector-of-bool approved these changes Oct 21, 2025

View reviewed changes

eramongodb added 2 commits October 21, 2025 14:42

Merge remote-tracking branch 'upstream/master' into HEAD

5160285

Use qualified explicit template specializations

3d276c8

eramongodb added 2 commits October 21, 2025 14:42

Remove extra doclines

42cac6f

Apply clangd IWYU fixes

33c1853

eramongodb mentioned this pull request Oct 21, 2025

Use qualified explicit template specializations #1486

Merged

Add missing source_errc enumerator to CATCH_REGISTER_ENUM

4ae0cd9

eramongodb merged commit e5d59c0 into mongodb:master Oct 21, 2025
2 of 3 checks passed

eramongodb deleted the cxx-abi-v1-instance branch October 21, 2025 20:15

eramongodb mentioned this pull request Oct 21, 2025

Use private virtual key_function() to address -Wweak-vtables #1487

Merged

		// @returns The exit code of the subprocess (`*is_signal` is `false`) or the signal used to kill the subprocess
		// (`*is_signal` is `true`) .


		} // namespace

		// mongocxx::v1::logger must not unnecessarily mpose special requirements on derived classes.

	/// Calls [`mongoc_init()`](https://mongoc.org/libmongoc/current/mongoc_cleanup.html).
	/// Calls [`mongoc_cleanup()`](https://mongoc.org/libmongoc/current/mongoc_cleanup.html).

		static_assert(!std::is_move_constructible<instance>::value, "bsoncxx::v1::instance must be non-moveable");
		static_assert(!std::is_copy_constructible<instance>::value, "bsoncxx::v1::instance must be non-copyable");

	# https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html
	# https://gcc.gnu.org/projects/cxx-status.html
	# https://gcc.gnu.org/releases.html
	if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1")
	message(FATAL_ERROR "GCC 8.1 or newer is required")
	endif()

CXX-3320 migrate instance and logger to mongocxx::v1 #1469

CXX-3320 migrate instance and logger to mongocxx::v1 #1469

Uh oh!

Conversation

eramongodb commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eramongodb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vector-of-bool left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eramongodb commented Sep 26, 2025 •

edited

Loading