-
Notifications
You must be signed in to change notification settings - Fork 457
[CDRIVER-6048][CDRIVER-4526] Add New Time and Duration Functionality #2074
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
vector-of-bool
merged 70 commits into
mongodb:master
from
vector-of-bool:CDRIVER-6048-time-types
Aug 15, 2025
Merged
[CDRIVER-6048][CDRIVER-4526] Add New Time and Duration Functionality #2074
vector-of-bool
merged 70 commits into
mongodb:master
from
vector-of-bool:CDRIVER-6048-time-types
Aug 15, 2025
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
This is intended to replace `mcd_duration` as a more capable duration type.
This uses preprocessor trickery to support more terse conversion and arithmetic of duration types.
The following changes are made, in order of significance: 1. The `mongoc_stream_poll` function declaration now says the units on its timeout. 2. Add a `_mongoc_stream_poll_internal` that takes a deadline timer rather than a timeout duration. 3. Move async-command typedefs into the async-command header. 4. async command creation now takes typed durations instead of integers 5. Async command internally uses timers, time points, and durations, rather than juggling `int64_t` for everything. 7. Heavy comments in async-cmd files are added all over the place to explain what is going on.
eramongodb
reviewed
Aug 14, 2025
kevinAlbs
approved these changes
Aug 14, 2025
Collaborator
kevinAlbs
left a comment
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 with a possible overflow check.
eramongodb
approved these changes
Aug 14, 2025
Contributor
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.
Some unaddressed feedback remaining (+ ClangFormat); otherwise, LGTM.
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.
Refer: CDRIVER-6048
Background
This is a preliminary work for CSOT, to give us a stable foundation for working with time going forward.
The C driver has had some duration/time types (
mcd_time_pointandmcd_duration), but they weren't widely used in the codebase and could use some TLC. This PR description will only present a high-level overview, as much of the implementation explanation has been added as commentary within the source files themselves.This PR also updates one core internal submodule to use the new time APIs, as a test-run of deploying them into the codebase in preparation for CSOT.
Fortunately, the vast majority of code additions in this changeset are explanatory comments.
New Time APIs
The new time types are similar to the previous
mcd_time types:struct mlib_durationrepresents a (possibly negative) difference between two points in time, with a fixed resolution (currently it has microsecond granularity). This is meant to replace any plain integers in the codebase that represent durations as a count of units. Using plain integers is problematic since the associated time unit cannot be statically enforced, and doing arithmetic on plain integers is inherently unsafe. Themlib_durationarithmetic is fully well-defined to use saturating arithmetic, so attempting to make a "too large" duration value will simply clamp to the nearest representable duration. While this is potentially problematic in extreme conditions, it only appears when juggling durations of hundreds of millennia, which reconnects Russia+Alaska and deletes Japan.struct mlib_time_pointrepresents a fixed point-in-time. Currently the implementation is written against the program's monotonic clock. It is encoded as anmlib_durationrelative to the unspecified monotonic clock epoch. Because the epoch is unspecified, this type cannot be reliably converted to any "IRL" time point. (I tried writing an lldb pretty-printer to do this for debugging purposes, but it turns out to be very non-trivial and platform-dependent.)mlib_now()which simply returns a time-point for the moment that the function is called.mlib_now()function actually replaces the implementation ofbson_get_monotonic_timefunction, which incidentally fixes CDRIVER-4526, sincemlib_nowuses the Win32 fine-grained monotonic clock.mlib_timerrepresents a deadline. This simply stores an "expires-at"mlib_time_point. The main reason for this being a separate type is to provide distinct types and functions specific to manipulating and inspecting deadlines.Creating and Manipulating Durations
The initial duration API was extremely verbose, and looked like:
while this works, its incredibly tedious to write. Instead, an
mlib_duration()function-like macro was added, which can be used used with two arguments to create a duration from a unit count, or three arguments to do duration arithmetic, and supports nested argument lists:See the doc comments in
mlib/duration.hfor an explanation. The macro trickery isn't pretty, but it is heavily commented for future maintainers.Usage in mongoc-async-cmd
The mongoc-async-cmd component was chosen arbitrarily as a test bed for the new APIs. Unfortunately, the code in there was heavily under-commented and used non-descriptive struct member names, so a lot of effort was spent just deducing what it is actually trying to do with the dozen
int64_tvalues it was using for timeouts/timestamps/deadlines.A heavy amount of comments and API renames have been applied to the module, to ensure that we're doing what we expect, and so future refactors won't need to spend a whole 3+ days trying to deduce what everything means. This doesn't give me a great outlook on future CSOT refactor work, depending on whether all time-handling code is similarly obtuse or I just happened to pick a module that was particularly problematic.
Additionally, my refactor of mongoc-async-cmd code led to the discovery that command deadlines can be violated (up to 2x or possibly greater) because of a hack that was added for CDRIVER-1571. Since this is a core module underlying all command execution, refactors will be required to prevent this if we want a successful CSOT implementation. An inline function doc-comment explains the problem.
Review Order
Initially, there was attempt to do isolated commits in logical order, but it somewhat broke down towards the end. Instead, it is recommended to review the final changes in the following order:
mlib/platform.his added to just#includesome common platform headers "correctly", since that's actually non-trivial.mlib/duration.his the basis of all time functions.mlib/time_point.hbuilds upondurationto create points-in-time.mlib/timer.ha simpler file that adds deadline-specific functions.