-
Notifications
You must be signed in to change notification settings - Fork 771
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
[21314] Improve resilience against clock adjustments #5018
[21314] Improve resilience against clock adjustments #5018
Conversation
b5fad03
to
519524c
Compare
Is there anything I need to to in order to get CI results? |
Hi @ma30002000, thanks for your contribution. |
…high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <[email protected]>
Signed-off-by: Matthias Schneider <[email protected]>
Signed-off-by: Matthias Schneider <[email protected]>
Signed-off-by: Matthias Schneider <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
519524c
to
d4d2ba4
Compare
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.
First of all, thank you for your patience waiting for this to be reviewed.
I took the liberty of rebasing this PR, and added a commit to fix one build issue.
Except for the changes in SharedMemGlobal.hpp
, the changes look good to me.
Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
Signed-off-by: Miguel Company <[email protected]>
…stments. Signed-off-by: Miguel Company <[email protected]>
…checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91.
Manual CIs: |
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 green CI
@Mergifyio backport 2.14.x 2.10.x |
✅ Backports have been created
|
* Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <[email protected]> * Use steady_clock instead for system_clock for calculating timeouts Signed-off-by: Matthias Schneider <[email protected]> * Use correct clock's duration for duration_cast Signed-off-by: Matthias Schneider <[email protected]> * Use Time_t::now() Signed-off-by: Matthias Schneider <[email protected]> * Fix build. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataWriterImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataReaderImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on StatefulWriter. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Protect current_time_since_unix_epoch against clock adjustments. Signed-off-by: Miguel Company <[email protected]> * Revert "Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91. --------- Signed-off-by: Matthias Schneider <[email protected]> Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]> (cherry picked from commit ccc690c) # Conflicts: # include/fastdds/rtps/writer/StatefulWriter.h # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # src/cpp/utils/time_t_helpers.hpp
* Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <[email protected]> * Use steady_clock instead for system_clock for calculating timeouts Signed-off-by: Matthias Schneider <[email protected]> * Use correct clock's duration for duration_cast Signed-off-by: Matthias Schneider <[email protected]> * Use Time_t::now() Signed-off-by: Matthias Schneider <[email protected]> * Fix build. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataWriterImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataReaderImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on StatefulWriter. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Protect current_time_since_unix_epoch against clock adjustments. Signed-off-by: Miguel Company <[email protected]> * Revert "Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91. --------- Signed-off-by: Matthias Schneider <[email protected]> Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]> (cherry picked from commit ccc690c) # Conflicts: # include/fastdds/rtps/writer/StatefulWriter.h # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # src/cpp/utils/time_t_helpers.hpp
* Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <[email protected]> * Use steady_clock instead for system_clock for calculating timeouts Signed-off-by: Matthias Schneider <[email protected]> * Use correct clock's duration for duration_cast Signed-off-by: Matthias Schneider <[email protected]> * Use Time_t::now() Signed-off-by: Matthias Schneider <[email protected]> * Fix build. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataWriterImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataReaderImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on StatefulWriter. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Protect current_time_since_unix_epoch against clock adjustments. Signed-off-by: Miguel Company <[email protected]> * Revert "Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91. --------- Signed-off-by: Matthias Schneider <[email protected]> Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]> Signed-off-by: paxifaer <[email protected]>
* Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <[email protected]> * Use steady_clock instead for system_clock for calculating timeouts Signed-off-by: Matthias Schneider <[email protected]> * Use correct clock's duration for duration_cast Signed-off-by: Matthias Schneider <[email protected]> * Use Time_t::now() Signed-off-by: Matthias Schneider <[email protected]> * Fix build. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataWriterImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataReaderImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on StatefulWriter. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Protect current_time_since_unix_epoch against clock adjustments. Signed-off-by: Miguel Company <[email protected]> * Revert "Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91. --------- Signed-off-by: Matthias Schneider <[email protected]> Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]> (cherry picked from commit ccc690c) # Conflicts: # include/fastdds/rtps/writer/StatefulWriter.h # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # src/cpp/utils/time_t_helpers.hpp
* Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <[email protected]> * Use steady_clock instead for system_clock for calculating timeouts Signed-off-by: Matthias Schneider <[email protected]> * Use correct clock's duration for duration_cast Signed-off-by: Matthias Schneider <[email protected]> * Use Time_t::now() Signed-off-by: Matthias Schneider <[email protected]> * Fix build. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataWriterImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataReaderImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on StatefulWriter. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Protect current_time_since_unix_epoch against clock adjustments. Signed-off-by: Miguel Company <[email protected]> * Revert "Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91. --------- Signed-off-by: Matthias Schneider <[email protected]> Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]> (cherry picked from commit ccc690c) # Conflicts: # include/fastdds/rtps/writer/StatefulWriter.h # src/cpp/fastdds/publisher/DataWriterImpl.cpp # src/cpp/fastdds/subscriber/DataReaderImpl.cpp # src/cpp/utils/time_t_helpers.hpp
* Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl) Signed-off-by: Matthias Schneider <[email protected]> * Use steady_clock instead for system_clock for calculating timeouts Signed-off-by: Matthias Schneider <[email protected]> * Use correct clock's duration for duration_cast Signed-off-by: Matthias Schneider <[email protected]> * Use Time_t::now() Signed-off-by: Matthias Schneider <[email protected]> * Fix build. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataWriterImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on DataReaderImpl. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Refactor on StatefulWriter. Signed-off-by: Miguel Company <[email protected]> * Refs #21314. Protect current_time_since_unix_epoch against clock adjustments. Signed-off-by: Miguel Company <[email protected]> * Revert "Use steady_clock instead of high_resolution_clock for status checks (high_resolution_clock might not be steady depending on STL impl)" This reverts commit d69eb91. --------- Signed-off-by: Matthias Schneider <[email protected]> Signed-off-by: Miguel Company <[email protected]> Co-authored-by: Miguel Company <[email protected]> (cherry picked from commit ccc690c)
Hi, sorry for the late response, could you please provide some more details on why you have not included the change of std::chrono::high_resolution_clock to std::chrono::steady_clock? Using a (on some systems) non-steady clock for timeout checks will have undesired effects. I also do not see a drawback of using steady_clock instead. |
@ma30002000 Because the changes affected the PortNode structure, which is part of the Shared Memory port segment. This means the changes would break interoperability with previous versions, which we should not do on a patch release. |
@MiguelCompany I understand - how and when can we address the issue in the future then? Unfortunately this change did not make it into 3.x... |
Description
When investigating a system undergoing larger system clock adjustments, I noticed that in some places, std::chrono::system_clock and std::chrono::high_resolution_clock are used for handling timeouts and status check intervals.
However, std::chrono::system_clock is definitely not steady, and std::chrono::high_resolution_clock is not steady quite often (see cppreference).
When undergoing clock adjustments (manually or due to clock server synchronization), timeouts and status checks might no longer be triggered when relying on timestamps based on std::chrono::system_clock.
@Mergifyio backport 2.14.x 2.10.x
Contributor Checklist
versions.md
file (if applicable).Reviewer Checklist