[SERVER-52904] Improve implementation of mongo::Status Created: 17/Nov/20  Updated: 29/Oct/23  Resolved: 23/Aug/21

Status: Closed
Project: Core Server
Component/s: Internal Code
Affects Version/s: None
Fix Version/s: 5.1.0-rc0

Type: Improvement Priority: Major - P3
Reporter: Billy Donahue Assignee: Billy Donahue
Resolution: Fixed Votes: 0
Labels: carry-over
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
depends on SERVER-55056 benchmark the performance of the Stat... Closed
depends on SERVER-52941 Break checked_cast.h assert_util.h cy... Closed
Related
is related to SERVER-52741 Avoid invariant when trying to get op... Closed
Backwards Compatibility: Fully Compatible
Sprint: Service arch 2020-12-28, Service Arch 2021-03-08, Service Arch 2021-03-22, Service Arch 2021-04-05, Service Arch 2021-04-19, Service Arch 2021-05-17, Service Arch 2021-05-31, Service Arch 2021-06-14, Service Arch 2021-06-28, Service Arch 2021-07-12, Service Arch 2021-08-09, Service Arch 2021-08-23, Service Arch 2021-09-20
Participants:
Story Points: 1

 Description   

Status has a lot of old code that needs a facelift.

(started as follow-on work from SERVER-52741)

Use boost::intrusive_ptr instead of handrolled refcounting for _error member.
This eliminates the need for copy and move ctors, assigns, and the destructor.

Don't declare operator<< for StringBuilderImpl that we don't define.

Inline the ubiquitous Status::OK() function.
Inline most constructors. Usually they forward to another constructor.

Define inlines inside the class consistently. This was previously mixed and inconsistent. It eliminates a lot of the boilerplate from out-of-class definitions.

Pass reason by std::string value when possible to save string copies!

Accept anything as a reason parameter if it can direct-initialize a std::string. This eliminates fwd decl of std::stream, and eliminates the combinatoric redundancy of providing overloads for const char*, StringData, std::string, and std::stream, which also leaves a lot of viable argument types missing anyway.

Remove useless const from return types and from parameters.

Use std::is_..._v traits bools.

Define inline-friend-defs for all == and != operators so they are applied symmetrically, and are only visible to ADL search.

Allocate the empty reason() static local string so it is valid at shutdown.

Remove the refCount() member, replace with a testOnlyAccess() proxy object that can retrieve the same data, but more obviously and strictly test only.

Simplify ErrorInfo to a simple struct, inherit from boost::intrusive_ref_counter. No explicit atomics for refcounting, no const members, no member functions. Let Status enforce the constness and creation invariants. Status::ErrorInfo becomes just plain info.

Add static Status::_createErrorInfo and Status::_parseErrorInfo functions to produce intrusive_ptr to initialize the _error member from constructors. Get rid of the risky *this = someStatus... error handling in favor of these functions.

https://mongodbcr.appspot.com/709190050/



 Comments   
Comment by Vivian Ge (Inactive) [ 06/Oct/21 ]

Updating the fixversion since branching activities occurred yesterday. This ticket will be in rc0 when it’s been triggered. For more active release information, please keep an eye on #server-release. Thank you!

Comment by Billy Donahue [ 23/Aug/21 ]

This change succeeded as long as I didn't try to refactor the RefCountable class.
Not sure what the problem was but changing the structure of RefCountable in what should have been a compatible way caused a null pointer dereference somewhere in mongo::ExpressionNary::_doAddDependencies(mongo::DepsTracker*) const during the `test_api_version_compatibility` task on RHEL80.

https://evergreen.mongodb.com/lobster/evergreen/task/mongodb_mongo_master_enterprise_rhel_80_64_bit_dynamic_required_test_api_version_compatibility_patch_05a7e410d067261ed8e940b7aa086a47502f45d1_611596113627e074e773072e_21_08_13_19_30_09/0/task#bookmarks=0%2C1984&l=1&shareLine=1536

This may be worth looking into separately!

Comment by Githook User [ 23/Aug/21 ]

Author:

{'name': 'Billy Donahue', 'email': 'billy.donahue@mongodb.com', 'username': 'BillyDonahue'}

Message: SERVER-52904 Status facelift
Branch: master
https://github.com/mongodb/mongo/commit/c0a7fd4ea328cea80eb81444f4cc480b508d949c

Comment by Billy Donahue [ 14/Jun/21 ]

Code review https://mongodbcr.appspot.com/762610002

Comment by Billy Donahue [ 19/Nov/20 ]

Marking this as depending on SERVER-52941 so we can use checked_pointer_cast in status.h while we're at it.

Comment by Billy Donahue [ 17/Nov/20 ]

code review - https://mongodbcr.appspot.com/709190050/ - (obsolete)

Generated at Thu Feb 08 05:29:20 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.