-
Type:
Improvement
-
Resolution: Unresolved
-
Priority:
Major - P3
-
None
-
Affects Version/s: None
-
Component/s: None
-
Service Arch
There may be a concerning trend toward storing precomputed Status objects in ordinary request processing code, and it should be addressed.
Ostensibly this is done for performance reasons but benchmarking shows that under contention on the shared object this may not materialize a net benefit (branched from close consideration of SERVER-53623).
There are often correctness issues involving the Status object's lifetime or its linkage violating the OneDefinitionRule.
A quick extremely loose grep found 20+ of these "bad" static status definitions. They definitely outnumber the safe definitions. Maybe there are 3x-5x this many if we looked closer. A quick grep, and basically all of these are unsafe for one reason or another.
src/mongo/db/auth/authorization_manager.h:110: static const Status authenticationFailedStatus; src/mongo/db/commands.h:652: static const Status kReadConcernNotSupported{ErrorCodes::InvalidOptions, src/mongo/db/commands.h:654: static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, src/mongo/db/commands.h:854: static const Status kReadConcernNotSupported{ErrorCodes::InvalidOptions, src/mongo/db/commands.h:856: static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, src/mongo/db/commands/count_cmd.cpp:101: static const Status kSnapshotNotSupported{ErrorCodes::InvalidOptions, src/mongo/db/commands/dbhash.cpp:93: static const Status kReadConcernNotSupported{ErrorCodes::InvalidOptions, src/mongo/db/commands/dbhash.cpp:95: static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, src/mongo/db/commands/map_reduce_command_base.h:55: static const Status kReadConcernNotSupported{ErrorCodes::InvalidOptions, src/mongo/db/commands/map_reduce_command_base.h:57: static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, src/mongo/db/commands/txn_cmds.cpp:159:static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, src/mongo/executor/connection_pool.h:94: static const Status kConnectionStateUnknown; src/mongo/executor/task_executor.h:84: static const inline Status kCallbackCanceledErrorStatus{ErrorCodes::CallbackCanceled, src/mongo/s/catalog_cache_loader_mock.h:98: static const Status kCollectionInternalErrorStatus; src/mongo/s/catalog_cache_loader_mock.h:99: static const Status kChunksInternalErrorStatus; src/mongo/s/catalog_cache_loader_mock.h:100: static const Status kDatabaseInternalErrorStatus; src/mongo/s/commands/cluster_abort_transaction_cmd.cpp:45:static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, src/mongo/s/commands/cluster_count_cmd.cpp:70: static const Status kSnapshotNotSupported{ErrorCodes::InvalidOptions, src/mongo/transport/session.h:79: static const Status ClosedStatus; src/mongo/transport/transport_layer.h:79: static const Status SessionUnknownStatus; src/mongo/transport/transport_layer.h:80: static const Status ShutdownStatus; src/mongo/transport/transport_layer.h:81: static const Status TicketSessionUnknownStatus;
They might not be achieving the efficiency goal they were intended to achieve, so we can just do the simpler thing return Status{code, msg}; with a clear conscience.