-
Notifications
You must be signed in to change notification settings - Fork 547
Replace scoped_bson_t with scoped_bson and scoped_bson_view #1470
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevinAlbs
reviewed
Oct 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I very much like the new design of scoped_bson
and scoped_bson_view
. One question about possibly throwing.
connorsmacd
reviewed
Oct 3, 2025
connorsmacd
approved these changes
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
kevinAlbs
approved these changes
Oct 3, 2025
This was referenced Oct 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to CXX-3320. This PR introduces a new helper utility for internal use only to manage
bson_t
objects and the creation of BSON documents for internal implementation:mongocxx::scoped_bson
andmongocxx::scoped_bson_view
(a lastd::string
andstd::string_view
). These two classes replacemongocxx::libbson::scoped_bson_t
(note: not to be confused withscoped_bson_value
forbson_value_t
). The v_noabi implementation is refactored to use the newscoped_bson(_view)
utilities to verify its correctness + behavior in advance of its widespread use inmongocxx::v1
implementations.The
scoped_bson_t
class is very old, going back all the way to the first commit (Jan 2015). Unfortunately it has been confusing and difficult to use due to preserving too muchbson_t
idiosyncracies in its API, in particular its handling of the"uninitializedbson_t
" state (e.g..init_from_static()
). Its initially simple and primitive API has been expanded over the years to support integration with bsoncxx and mongocxx API, often in tension withbson_t
behaviors (e.g. owning vs. non-owning and uninitialized vs. empty vs. moved-from). This PR proposes an alternative API which abandons the "uninitializedbson_t
" state entirely and always preserves bsoncxx-idiomatic well-defined state.First, both
scoped_bson
andscoped_bson_view
completely avoid depending onbson_t
for ownership or managment of underlying BSON data. No operations depend on thebson_t
aside from exposingbson_t const*
for mongoc API integration. Instead, thebsoncxx::v1::document::value
orbsoncxx::v1::document::view
data member always own and manage associated BSON data. Thebson_t
data member is always and only ever initialized withbson_init_static()
viathis->sync_bson()
after the bsoncxx value/view object has been updated first. Any applicable "would-be-uninitialized", "null", or "invalid" states are represented as an "invalid" document (.data() == nullptr
) per v1 API's new well-defined "invalid" state forvalue
andview
.Important
There is no "uninitialized
bson_t
" state exposed by thescoped_bson(_view)
API.Second, the owning
scoped_bson
class does its best to avoid unnecessary allocations. This includes usingbson_destroy_with_steal()
to acquire ownership of BSON data frombson_t*
arguments, (re)usingbsoncxx::v1::document::view
's special empty state whenever an empty document is detected to avoid allocating emptybson_t
objects, and releasing ownership of underlying BSON data asbsoncxx::v1::document::value
with rvalue overloads.Third,
scoped_bson(_view)
provide.out_ptr()
and.inout_ptr()
to interface with mongoc API that requiresbson_t*
andbson_t const**
arguments. This pattern is inspired by P1132R8, although it has been greatly simplified to suit our limited purposes. This ensures thescoped_bson(_view)
object always remains in a valid, well-defined state regardless of the result of the mongoc API call.Last,
scoped_bson
implements anoperator+=
overload to minimally yet efficiently support building complex BSON documents in v1 implementations without depending on the v_noabi builder API. The "concatenation" operation is ultimately the only builder operation needed to build internal BSON documents: all other BSON builder behavior can be handled by theBCON_*
family of macros, whose resultingbson_t*
is (typically) immediately used to initialize ascoped_bson
object:This will be the primary method by which the v1 implementation constructs BSON documents for internal use until a new public BSON builder API is implemented with CXX-3275. A relaxed extended JSON constructor is also implemented (e.g.
scoped_bson{R"("x": 1)"}
) , but it is primarily for test convenience only. Implementation code should prefer to avoid string parsing performance overhead and should prefer to be explicit about the types of BSON element being constructed.Note
Update: this problem is documented by CDRIVER-6113.
There was an attempt to default the
scoped_bson_view
copy SMFs and thescoped_bson
move SMFs. This should be possible in theory givenbson_t
is only ever a view to the accompanyingvalue
orview
object's underlying BSON data. However, this led very difficult-to-diagnose undefined behavior. Resulting spurious segmentation faults include stack traces pointing tostd::function<void (unsigned char*)>::function(std::function<void (unsigned char*)>&&)
withthis=0.0
or the__memcmp_avx2_movbe
intrinsic operation (???). These segfaults were unobservable when sanitizers are enabled (?!). Neither_DFORTIFY_SOURCE=3
norGLIBCXX_ASSERTIONS
were able to identify any problems. My takeaway is thatbson_t
is not trivially-copyable even when only ever zero-initialized or initialized withbson_init_static()
.