Skip to content
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

Fix wrong TimeUtils::currentTimeMillis() implementation #387

Merged

Conversation

BewareMyPower
Copy link
Contributor

Motivation

TimeUtils::currentTimeMillis() is used to get the timestamp since 1970 but high_resolution_clock does not necessarily represent the system clock. For example, given the following code:

template <typename Clock>
inline int64_t getTimestamp() {
    using namespace std::chrono;
    return duration_cast<milliseconds>(Clock::now().time_since_epoch()).count();
}

int main() {
    std::cout << time(nullptr) << std::endl;
    std::cout << getTimestamp<std::chrono::system_clock>() << std::endl;
    std::cout << getTimestamp<std::chrono::high_resolution_clock>() << std::endl;
}

The outputs could be:

1705584279
1705584279351
38566832

Modifications

Use std::system_clock for currentTimeMillis(). Add BackoffTest.testCurrentTimeMillis to verify the result is similar to 1000 times of time(nullptr) (the timestamp in seconds from the C API).

### Motivation

`TimeUtils::currentTimeMillis()` is used to get the timestamp since
1970 but `high_resolution_clock` does not necessarily represent the
system clock. For example, given the following code:

```c++
template <typename Clock>
inline int64_t getTimestamp() {
    using namespace std::chrono;
    return duration_cast<milliseconds>(Clock::now().time_since_epoch()).count();
}

int main() {
    std::cout << time(nullptr) << std::endl;
    std::cout << getTimestamp<std::chrono::system_clock>() << std::endl;
    std::cout << getTimestamp<std::chrono::high_resolution_clock>() << std::endl;
}
```

The outputs could be:

```
1705584279
1705584279351
38566832
```

### Modifications

Use `std::system_clock` for `currentTimeMillis()`. Add
`BackoffTest.testCurrentTimeMillis` to verify the result is similar to
1000 times of `time(nullptr)` (the timestamp in seconds from the C API).
@BewareMyPower BewareMyPower added the bug Something isn't working label Jan 18, 2024
@BewareMyPower BewareMyPower self-assigned this Jan 18, 2024
@BewareMyPower
Copy link
Contributor Author

+ echo 'Building ZLib 1.2.12'
+ curl -O -L https://zlib.net/fossils/zlib-1.2.12.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 20429    0 20429    0     0   177k      0 --:--:-- --:--:-- --:--:--  178k
+ tar zxf zlib-1.2.12.tar.gz
tar: Error opening archive: Unrecognized archive format

The error is not related to this PR. So I merged it first. I will take a look at this issue later.

@BewareMyPower BewareMyPower merged commit 6eb228e into apache:main Jan 19, 2024
11 of 12 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/fix-timestamp branch January 19, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants