Skip to content

Conversation

@jureviciusr
Copy link
Contributor

The connection timeout in the connection class was using the std::chrono::system_clock which is not monotonic and susceptible to time jumps when system wall clock is adjusted, see here: https://en.cppreference.com/w/cpp/chrono/system_clock

This can lead to connection timeout incorrectly evaluated. std::chrono::steady_clock should be used instead, as it is not susceptible for wall clock adjustments and should be preferred for time duration measurements: https://en.cppreference.com/w/cpp/chrono/steady_clock

The PR changes are the following:

  • Remove the millis() public method from utils
  • Replace integer millisecond with std::chrono::milliseconds for a more explicit type
  • Use steady_clock to sample timestamps to measure connection timeout

@jureviciusr jureviciusr requested a review from KonradRudin May 16, 2025 08:39
@jureviciusr jureviciusr self-assigned this May 16, 2025
@jureviciusr jureviciusr requested a review from DanMesh May 16, 2025 13:14
Comment on lines -103 to -106
inline uint64_t millis() noexcept {
return std::chrono::duration_cast<std::chrono::milliseconds>(
std::chrono::system_clock::now().time_since_epoch()).count();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would hope it's safe to remove this public helper function.

If we're worried, we could keep and at least make it return a steady clock time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how far reaching is this, within this project it was only used only once. We can deprecate the method first, so it would print a compile-time warning with a clear message and get deleted with next version bump, e.g.

    [[deprecated("millis() method uses non-monotonic clock source and it will be removed in the next release.")]] inline uint64_t millis() noexcept {
        return std::chrono::duration_cast<std::chrono::milliseconds>(
                std::chrono::system_clock::now().time_since_epoch()).count();
    }

I would strongly suggest to remove it, since it's up to the developer to select an appropriate clock source for the task.

@jureviciusr jureviciusr merged commit 785a6c0 into main May 16, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants