Skip to content

CXX-3065 guard against use of REQUIRE within APM callbacks #1171

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

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

eramongodb
Copy link
Contributor

Summary

Resolves CXX-3065. Verified by this patch.

Followup to #1015:

Any user code that currently violates the nothrow requirement will now terminate via noexcept rather than be allowed to continue with potentially unbounded libmongoc runtime errors. [...] (consider: our own test cases violate this requirement in failure conditions, as Catch2 uses exceptions to report test assertion failures))

This PR implements a workaround for such assertions in our test suite.

Exceptions in APM Callbacks

The "[sdam_monitoring]" test cases (two total) can be used to demonstrate the problem by deliberately inverting one of the assertion conditions to trigger assertion failure:

--- a/src/mongocxx/test/sdam-monitoring.cpp
+++ b/src/mongocxx/test/sdam-monitoring.cpp
@@ ... @@ TEST_CASE("Heartbeat failed event", "[sdam_monitoring]") {
     // ServerHeartbeatFailedEvent
     apm_opts.on_heartbeat_failed([&](const events::heartbeat_failed_event& event) {
         heartbeat_failed_events++;
-        REQUIRE_FALSE(event.host().empty());
+        REQUIRE(event.host().empty()); // Deliberate assertion failure.
         REQUIRE_FALSE(event.message().empty());
         REQUIRE(event.port() != 0);
         REQUIRE(!event.awaited());

Executing test_driver -r compact --order rand --rng-seed 0 "[sdam_monitoring]" produces the following output (slightly edited for brevity):

sdam-monitoring.cpp: failed: event.host().empty() for: false
fatal error: APM callback heartbeat_failed exited via an exception
terminate called after throwing an instance of 'Catch::TestFailureException'
sdam-monitoring.cpp: failed: fatal error condition with message: 'SIGABRT - Abort (abnormal termination) signal'; expression was: {Unknown expression after the reported line}
Failed 1 test case, failed both 2 assertions.

The test suite prematurely terminates after running only the "Heartbeat failed event" test case due to the QoI check added in #1015. The "SDAM Monitoring" test case is not executed.

Exception Guards

A new bsoncxx/test/exception_guard.hh component is added (for testing only!) providing a mechanism to store-and-rethrow exceptions thrown within an "exception guard region" to allow for graceful return from APM callback functions while still propagating the thrown exception.

This feature is achieved using std::exception_ptr:

mongocxx::options::apm apm_opts;

// Exception Guard State
std::exception_ptr eptr = nullptr;

apm_opts.on_command_started([&](auto event) {
  // Exception Guard Region: Begin
  try {

    REQUIRE(false); // Throw a Catch::TestAssertFailure.

  } catch (...) {
    if (!eptr) {
      eptr = std::current_exception(); // Catch and store the exception.
    }
  }
  // Exception Guard Region: End
});

// ...

// Exception Guard Check
if (eptr) {
  std::rethrow_exception(eptr); // Rethrow the exception.
}

This pattern is abstracted by exception guard macros:

mongocxx::options::apm apm_opts;

bsoncxx::test::exception_guard_state eguard;

apm_opts.on_command_started([&](auto event) {
  BSONCXX_TEST_EXCEPTION_GUARD_BEGIN(eguard);

  REQUIRE(false); // Throw a Catch::TestAssertFailure.

  BSONCXX_TEST_EXCEPTION_GUARD_END(eguard); // Catch and store the exception.
});

// ...

BSONCXX_TEST_EXCEPTION_GUARD_CHECK(eguard); // Rethrow the exception.

An additional BSONCXX_TEST_EXCEPTION_GUARD_RESET() macro is provided to reset the exception guard state for testing and reuse (this is not expected to be used often).

The utility includes some additional features such as basic thread-safety and logging of ignored exceptions (only the first exception is caught and checked). See the new test code for a demonstration of its features.

Supporting the storage of multiple exceptions was considered overkill considering the purpose this utility is meant to fulfill (a workaround specifically for exceptions expected to be thrown from APM callbacks). It is not expected to be used for other purposes.

Note

Catch2 is not thread-safe. This utility does not add support thread-safe assertions.

BSONCXX_TEST_EXCEPTION_GUARD_CHECK() is not strictly necessary (since Catch tracks test case failure internally regardless of how the exception is handled), but helps avoid the unnecessary and noisy execution of further assertions for an already-failed test case. The check also provides an opportunity to log any additionally-thrown exceptions which were ignored by the exception guard.

Applying this pattern to the "[sdam_monitoring]" case described above produces the following output:

../src/mongocxx/test/sdam-monitoring.cpp:291: failed: event.host().empty() for: false
Failed 1 test case, failed 1 assertion.

Importantly, there is no premature termination, and although it is not apparent from this compact output, the other "SDAM Monitoring" test case is executed with success. The QoI check is no longer triggered.

CHECK vs. REQUIRE

In most cases, this new utility is not used. As noted in #1015 (review), CHECK is a non-throwing alternative to REQUIRE which still marks the test case as failed. This PR favors changing REQUIRE* -> CHECK* when able over using the new exception guard utility, as this is arguably the more "correct" use of APM callbacks (which, again, shouldn't be throwing exceptions).

However, in the interest of avoiding significant refactors, this is only done in obvious cases where the assertion does not appear to be guarding code (e.g. a null check prior to dereference). This PR does not attempt to eliminate REQUIRE entirely from APM callbacks.

Miscellaneous

  • All test code should be including <bsoncxx/test/catch.hh> (which defines StringMaker specializations for our types) instead of directly including the Catch2 header.
  • Added SYSTEM to the target_include_directories() command importing mnmlstc/core headers to avoid irrelevant warnings-as-errors.

@eramongodb eramongodb requested a review from kevinAlbs July 18, 2024 21:19
@eramongodb eramongodb self-assigned this Jul 18, 2024
@eramongodb eramongodb merged commit ae07bd2 into mongodb:master Jul 22, 2024
70 of 78 checks passed
@eramongodb eramongodb deleted the cxx-3065 branch July 22, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants