From 32c4765e7a29ebbb7470a944dabf70d69134a486 Mon Sep 17 00:00:00 2001 From: Billy Donahue Date: Thu, 19 Nov 2020 17:22:13 +0000 Subject: SERVER-50684 rename,cleanup internalAssert->iassert --- docs/exception_architecture.md | 94 +++++++++++++------------- src/mongo/db/operation_cpu_timer.cpp | 4 +- src/mongo/db/service_entry_point_common.cpp | 6 +- src/mongo/db/wire_version.cpp | 3 +- src/mongo/transport/service_executor.h | 4 +- src/mongo/transport/service_executor_fixed.cpp | 2 +- src/mongo/util/assert_util.cpp | 2 +- src/mongo/util/assert_util.h | 31 +++------ src/mongo/util/assert_util_test.cpp | 8 +-- src/mongo/util/interruptible.h | 4 +- 10 files changed, 72 insertions(+), 86 deletions(-) diff --git a/docs/exception_architecture.md b/docs/exception_architecture.md index 105319774f3..d5324a6cef3 100644 --- a/docs/exception_architecture.md +++ b/docs/exception_architecture.md @@ -1,7 +1,7 @@ # Exception Architecture MongoDB code uses the following types of assertions that are available for use: -- `uassert` and `internalAssert` +- `uassert` and `iassert` - Checks for per-operation user errors. Operation-fatal. - `tassert` - Like uassert, but inhibits clean shutdown. @@ -24,21 +24,21 @@ The following types of assertions are deprecated: - `dassert` - Calls `invariant` but only in debug mode. Do not use! -MongoDB uses a series of `ErrorCodes` (defined in [mongo/base/error_codes.yml][error_codes_yml]) to -identify and categorize error conditions. `ErrorCodes` are defined in a YAML file and converted to -C++ files using [MongoDB's IDL parser][idlc_py] at compile time. We also use error codes to create -`Status` objects, which convey the success or failure of function invocations across the code base. -`Status` objects are also used internally by `DBException`, MongoDB's primary exception class, and -its children (e.g., `AssertionException`) as a means of maintaining metadata for exceptions. The +MongoDB uses a series of `ErrorCodes` (defined in [mongo/base/error_codes.yml][error_codes_yml]) to +identify and categorize error conditions. `ErrorCodes` are defined in a YAML file and converted to +C++ files using [MongoDB's IDL parser][idlc_py] at compile time. We also use error codes to create +`Status` objects, which convey the success or failure of function invocations across the code base. +`Status` objects are also used internally by `DBException`, MongoDB's primary exception class, and +its children (e.g., `AssertionException`) as a means of maintaining metadata for exceptions. The proper usage of these constructs is described below. ## Considerations When per-operation invariant checks fail, the current operation fails, but the process and -connection persist. This means that `massert`, `uassert`, `internalAssert` and `verify` only -terminate the current operation, not the whole process. Be careful not to corrupt process state by -mistakenly using these assertions midway through mutating process state. Examples of this include -`uassert`, `internalAssert` and `massert` inside of constructors and destructors. +connection persist. This means that `massert`, `uassert`, `iassert` and `verify` only +terminate the current operation, not the whole process. Be careful not to corrupt process state by +mistakenly using these assertions midway through mutating process state. Examples of this include +`uassert`, `iassert` and `massert` inside of constructors and destructors. `fassert` failures will terminate the entire process; this is used for low-level checks where continuing might lead to corrupt data or loss of data on disk. @@ -49,13 +49,13 @@ invoke the tripwire fatal assertion. This is useful for ensuring that operation a test suite to fail, without resorting to different behavior during testing, and without allowing user operations to potentially disrupt production deployments by terminating the server. -Both `massert` and `uassert` take error codes, so that all assertions have codes associated with -them. Currently, programmers are free to provide the error code by either using a unique location -number or choose from existing `ErrorCodes`. Unique location numbers are assigned incrementally and +Both `massert` and `uassert` take error codes, so that all assertions have codes associated with +them. Currently, programmers are free to provide the error code by either using a unique location +number or choose from existing `ErrorCodes`. Unique location numbers are assigned incrementally and have no meaning other than a way to associate a log message with a line of code. -`internalAssert` provides similar functionality to `uassert`, but it logs at a higher level and -does not increment user assertion counters. We should always choose `internalAssert` over `uassert` +`iassert` provides similar functionality to `uassert`, but it logs at a higher level and +does not increment user assertion counters. We should always choose `iassert` over `uassert` when we expect a failure, a failure might be recoverable, or failure accounting is not interesting. @@ -72,46 +72,46 @@ The inheritance hierarchy resembles: See util/assert_util.h. -Generally, code in the server should be able to tolerate (e.g., catch) a `DBException`. Server -functions must be structured with exception safety in mind, such that `DBException` can propagate -upwards harmlessly. The code should also expect, and properly handle, `UserException`. We use +Generally, code in the server should be able to tolerate (e.g., catch) a `DBException`. Server +functions must be structured with exception safety in mind, such that `DBException` can propagate +upwards harmlessly. The code should also expect, and properly handle, `UserException`. We use [Resource Acquisition Is Initialization][raii] heavily. ## ErrorCodes and Status -MongoDB uses `ErrorCodes` both internally and externally: a subset of error codes (e.g., -`BadValue`) are used externally to pass errors over the wire and to clients. These error codes are -the means for MongoDB processes (e.g., *mongod* and *mongo*) to communicate errors, and are visible -to client applications. Other error codes are used internally to indicate the underlying reason for -a failed operation. For instance, `PeriodicJobIsStopped` is an internal error code that is passed -to callback functions running inside a [`PeriodicRunner`][periodic_runner_h] once the runner is -stopped. The internal error codes are for internal use only and must never be returned to clients +MongoDB uses `ErrorCodes` both internally and externally: a subset of error codes (e.g., +`BadValue`) are used externally to pass errors over the wire and to clients. These error codes are +the means for MongoDB processes (e.g., *mongod* and *mongo*) to communicate errors, and are visible +to client applications. Other error codes are used internally to indicate the underlying reason for +a failed operation. For instance, `PeriodicJobIsStopped` is an internal error code that is passed +to callback functions running inside a [`PeriodicRunner`][periodic_runner_h] once the runner is +stopped. The internal error codes are for internal use only and must never be returned to clients (i.e., in a network response). - -Zero or more error categories can be assigned to `ErrorCodes`, which allows a single handler to -serve a group of `ErrorCodes`. `RetriableError`, for instance, is an `ErrorCategory` that includes -all retriable `ErrorCodes` (e.g., `HostUnreachable` and `HostNotFound`). This implies that an -operation that fails with any error code in this category can be safely retried. We can use -`ErrorCodes::isA<${category}>(${error})` to check if `error` belongs to `category`. Alternatively, -we can use `ErrorCodes::is${category}(${error})` to check error categories. Both methods provide + +Zero or more error categories can be assigned to `ErrorCodes`, which allows a single handler to +serve a group of `ErrorCodes`. `RetriableError`, for instance, is an `ErrorCategory` that includes +all retriable `ErrorCodes` (e.g., `HostUnreachable` and `HostNotFound`). This implies that an +operation that fails with any error code in this category can be safely retried. We can use +`ErrorCodes::isA<${category}>(${error})` to check if `error` belongs to `category`. Alternatively, +we can use `ErrorCodes::is${category}(${error})` to check error categories. Both methods provide similar functionality. -To represent the status of an executed operation (e.g., a command or a function invocation), we -use `Status` objects, which represent an error state or the absence thereof. A `Status` uses the -standardized `ErrorCodes` to determine the underlying cause of an error. It also allows assigning -a textual description, as well as code-specific extra info, to the error code for further -clarification. The extra info is a subclass of `ErrorExtraInfo` and specific to `ErrorCodes`. Look +To represent the status of an executed operation (e.g., a command or a function invocation), we +use `Status` objects, which represent an error state or the absence thereof. A `Status` uses the +standardized `ErrorCodes` to determine the underlying cause of an error. It also allows assigning +a textual description, as well as code-specific extra info, to the error code for further +clarification. The extra info is a subclass of `ErrorExtraInfo` and specific to `ErrorCodes`. Look for `extra` in [here][error_codes_yml] for reference. -MongoDB provides `StatusWith` to enable functions to return an error code or a value without -requiring them to have multiple outputs. This makes exception-free code cleaner by avoiding -functions with multiple out parameters. We can either pass an error code or an actual value to a -`StatusWith` object, indicating failure or success of the operation. For examples of the proper -usage of `StatusWith`, see [mongo/base/status_with.h][status_with_h] and -[mongo/base/status_with_test.cpp][status_with_test_cpp]. It is highly recommended to use `uassert` -or `internalAssert` over `StatusWith`, and catch exceptions instead of checking `Status` objects -returned from functions. Using `StatusWith` to indicate exceptions, instead of throwing via -`uassert` and `internalAssert`, makes it very difficult to identify that an error has occurred, and +MongoDB provides `StatusWith` to enable functions to return an error code or a value without +requiring them to have multiple outputs. This makes exception-free code cleaner by avoiding +functions with multiple out parameters. We can either pass an error code or an actual value to a +`StatusWith` object, indicating failure or success of the operation. For examples of the proper +usage of `StatusWith`, see [mongo/base/status_with.h][status_with_h] and +[mongo/base/status_with_test.cpp][status_with_test_cpp]. It is highly recommended to use `uassert` +or `iassert` over `StatusWith`, and catch exceptions instead of checking `Status` objects +returned from functions. Using `StatusWith` to indicate exceptions, instead of throwing via +`uassert` and `iassert`, makes it very difficult to identify that an error has occurred, and could lead to the wrong error being propagated. ## Gotchas diff --git a/src/mongo/db/operation_cpu_timer.cpp b/src/mongo/db/operation_cpu_timer.cpp index 34cb63eb3d4..b870e621909 100644 --- a/src/mongo/db/operation_cpu_timer.cpp +++ b/src/mongo/db/operation_cpu_timer.cpp @@ -60,8 +60,8 @@ Nanoseconds getThreadCPUTime() { struct timespec t; if (auto ret = clock_gettime(CLOCK_THREAD_CPUTIME_ID, &t); ret != 0) { int ec = errno; - internalAssert(Status(ErrorCodes::InternalError, - "Unable to get time: {}"_format(errnoWithDescription(ec)))); + iassert(Status(ErrorCodes::InternalError, + "Unable to get time: {}"_format(errnoWithDescription(ec)))); } return Seconds(t.tv_sec) + Nanoseconds(t.tv_nsec); } diff --git a/src/mongo/db/service_entry_point_common.cpp b/src/mongo/db/service_entry_point_common.cpp index 9f3fd1a22d5..528b37d071e 100644 --- a/src/mongo/db/service_entry_point_common.cpp +++ b/src/mongo/db/service_entry_point_common.cpp @@ -1702,7 +1702,7 @@ void ExecCommandDatabase::_handleFailure(Status status) { if (ErrorCodes::isA(status.code())) { // Rethrow the exception to the top to signal that the client connection should be closed. - internalAssert(status); + iassert(status); } } @@ -1852,7 +1852,7 @@ Future receivedCommands(std::shared_ptrgetOpCtx(); @@ -1870,7 +1870,7 @@ Future receivedCommands(std::shared_ptr(status.code())) { // Return the exception to the top to signal that the client connection should be // closed. - internalAssert(status); + iassert(status); } }) .then([execContext] { return makeCommandResponse(std::move(execContext)); }); diff --git a/src/mongo/db/wire_version.cpp b/src/mongo/db/wire_version.cpp index c0757324dbc..03fc8543a87 100644 --- a/src/mongo/db/wire_version.cpp +++ b/src/mongo/db/wire_version.cpp @@ -70,8 +70,7 @@ void WireSpec::reset(Specification spec) { BSONObj oldSpec, newSpec; { stdx::lock_guard lk(_mutex); - internalAssert( - ErrorCodes::NotYetInitialized, "WireSpec is not yet initialized", isInitialized()); + iassert(ErrorCodes::NotYetInitialized, "WireSpec is not yet initialized", isInitialized()); oldSpec = specToBSON(*_spec.get()); _spec = std::make_shared(std::move(spec)); diff --git a/src/mongo/transport/service_executor.h b/src/mongo/transport/service_executor.h index b702198e6b5..b5a87c3a77c 100644 --- a/src/mongo/transport/service_executor.h +++ b/src/mongo/transport/service_executor.h @@ -92,8 +92,8 @@ public: * for execution on the service executor. May throw if "scheduleTask" returns a non-okay status. */ void schedule(OutOfLineExecutor::Task func) override { - internalAssert(scheduleTask([task = std::move(func)]() mutable { task(Status::OK()); }, - ScheduleFlags::kEmptyFlags)); + iassert(scheduleTask([task = std::move(func)]() mutable { task(Status::OK()); }, + ScheduleFlags::kEmptyFlags)); } /* diff --git a/src/mongo/transport/service_executor_fixed.cpp b/src/mongo/transport/service_executor_fixed.cpp index b068e487126..d0b918cf598 100644 --- a/src/mongo/transport/service_executor_fixed.cpp +++ b/src/mongo/transport/service_executor_fixed.cpp @@ -160,7 +160,7 @@ Status ServiceExecutorFixed::scheduleTask(Task task, ScheduleFlags flags) { // May throw if an attempt is made to schedule after the thread pool is shutdown. try { _threadPool->schedule([task = std::move(task)](Status status) mutable { - internalAssert(status); + iassert(status); invariant(_executorContext); _executorContext->run(std::move(task)); }); diff --git a/src/mongo/util/assert_util.cpp b/src/mongo/util/assert_util.cpp index 7039a5c1162..3013402eac4 100644 --- a/src/mongo/util/assert_util.cpp +++ b/src/mongo/util/assert_util.cpp @@ -269,7 +269,7 @@ MONGO_COMPILER_NOINLINE void msgassertedWithLocation(const Status& status, error_details::throwExceptionForStatus(status); } -void internalAssertWithLocation(SourceLocationHolder loc, const Status& status) { +void iassertWithLocation(SourceLocationHolder loc, const Status& status) { if (status.isOK()) return; LOGV2_DEBUG(4892201, 3, "Internal assertion", "error"_attr = status, "location"_attr = loc); diff --git a/src/mongo/util/assert_util.h b/src/mongo/util/assert_util.h index b44dceccafa..fd66dff2699 100644 --- a/src/mongo/util/assert_util.h +++ b/src/mongo/util/assert_util.h @@ -437,38 +437,25 @@ inline void massertStatusOKWithLocation(const Status& status, const char* file, } /** - * `internalAssert` is provided as an alternative for `uassert` variants (e.g., `uassertStatusOK`) + * `iassert` is provided as an alternative for `uassert` variants (e.g., `uassertStatusOK`) * to support cases where we expect a failure, the failure is recoverable, or accounting for the - * failure, updating assertion counters, isn't desired. `internalAssert` logs at D3 instead of D1, + * failure, updating assertion counters, isn't desired. `iassert` logs at D3 instead of D1, * which helps with reducing the noise of assertions in production. The goal is to keep one - * interface (i.e., `internalAssert(...)`) for all possible assertion variants, and use function + * interface (i.e., `iassert(...)`) for all possible assertion variants, and use function * overloading to expand type support as needed. */ -#define internalAssert(...) \ - ::mongo::internalAssertWithLocation(MONGO_SOURCE_LOCATION(), __VA_ARGS__) +#define iassert(...) ::mongo::iassertWithLocation(MONGO_SOURCE_LOCATION(), __VA_ARGS__) -void internalAssertWithLocation(SourceLocationHolder loc, const Status& status); +void iassertWithLocation(SourceLocationHolder loc, const Status& status); -inline void internalAssertWithLocation(SourceLocationHolder loc, Status&& status) { - internalAssertWithLocation(std::move(loc), status); -} - -inline void internalAssertWithLocation(SourceLocationHolder loc, - int msgid, - const std::string& msg, - bool expr) { +inline void iassertWithLocation(SourceLocationHolder loc, int msgid, std::string msg, bool expr) { if (MONGO_unlikely(!expr)) - internalAssertWithLocation(std::move(loc), Status(ErrorCodes::Error(msgid), msg)); -} - -template -inline void internalAssertWithLocation(SourceLocationHolder loc, const StatusWith& sw) { - internalAssertWithLocation(std::move(loc), sw.getStatus()); + iassertWithLocation(std::move(loc), Status(ErrorCodes::Error(msgid), std::move(msg))); } template -inline void internalAssertWithLocation(SourceLocationHolder loc, StatusWith&& sw) { - internalAssertWithLocation(std::move(loc), sw); +void iassertWithLocation(SourceLocationHolder loc, const StatusWith& sw) { + iassertWithLocation(std::move(loc), sw.getStatus()); } /** diff --git a/src/mongo/util/assert_util_test.cpp b/src/mongo/util/assert_util_test.cpp index bb5a502a65d..a8af00a844a 100644 --- a/src/mongo/util/assert_util_test.cpp +++ b/src/mongo/util/assert_util_test.cpp @@ -233,13 +233,13 @@ TEST(AssertUtils, InternalAssertWithStatus) { auto userAssertions = assertionCount.user.load(); try { Status status = {ErrorCodes::BadValue, "Test"}; - internalAssert(status); + iassert(status); } catch (const DBException& ex) { ASSERT_EQ(ex.code(), ErrorCodes::BadValue); ASSERT_EQ(ex.reason(), "Test"); } - internalAssert(Status::OK()); + iassert(Status::OK()); ASSERT_EQ(userAssertions, assertionCount.user.load()); } @@ -247,13 +247,13 @@ TEST(AssertUtils, InternalAssertWithStatus) { TEST(AssertUtils, InternalAssertWithExpression) { auto userAssertions = assertionCount.user.load(); try { - internalAssert(48922, "Test", false); + iassert(48922, "Test", false); } catch (const DBException& ex) { ASSERT_EQ(ex.code(), 48922); ASSERT_EQ(ex.reason(), "Test"); } - internalAssert(48922, "Another test", true); + iassert(48922, "Another test", true); ASSERT_EQ(userAssertions, assertionCount.user.load()); } diff --git a/src/mongo/util/interruptible.h b/src/mongo/util/interruptible.h index 5b14c29bcc1..d1b8981adf3 100644 --- a/src/mongo/util/interruptible.h +++ b/src/mongo/util/interruptible.h @@ -317,7 +317,7 @@ public: * Raises a AssertionException if this operation is in a killed state. */ void checkForInterrupt() { - internalAssert(checkForInterruptNoAssert()); + iassert(checkForInterruptNoAssert()); } /** @@ -395,7 +395,7 @@ public: if (!swResult.isOK()) { _onWake(latchName, WakeReason::kInterrupt, speed); - internalAssert(std::move(swResult)); + iassert(std::move(swResult)); } if (pred()) { -- cgit v1.2.1