y2038: unify to uint64 GetTickCount64() to properly manage milliseconds - #702
Conversation
Addresses #602 Keep only uint64 GetTickCount64() as a source of ticks in milliseconds. Update all calls to use this function and to store values in uint64 datatypes. GetTickCount64() will use internally clock_gettime() instead of the deprecated gettimeofday(). Additionally, reduce calls by removing unused variables and too consecutive calls that can be stored.
|
Two notes on the GetTickCount rewrite — first is a bigger ask than nits, but I think worth doing while we're here: 1. Use You stayed on Looking at your diff, the callers split cleanly. Almost all of them are the
Those four sites should use Concrete change: // GetTickCount.cpp
uint64 GetTickCount64(void) {
struct timespec ts;
- // Fetch time (Y2038-safe)
- clock_gettime(CLOCK_REALTIME, &ts);
+ // CLOCK_MONOTONIC: tick count is for timeouts / deltas, must not
+ // jump when wall clock is adjusted. Callers that need a Unix
+ // timestamp (CClientCredits::SetLastSeen, partfile source-seeds
+ // serialization, wxCas defaults) should use time(NULL) instead.
+ clock_gettime(CLOCK_MONOTONIC, &ts);
msecs = (uint64) ts.tv_sec * 1000;
msecs += ts.tv_nsec / 1000000;
return msecs;
} // ClientCredits.cpp
- m_pCredits->nLastSeen = GetTickCount64()/1000;
+ m_pCredits->nLastSeen = time(NULL); // PartFile.cpp source-seeds write
- file.WriteUInt32((uint32)(GetTickCount64()/1000));
+ file.WriteUInt32((uint32) time(NULL)); // PartFile.cpp source-seeds 120-min validity check
- if ((time + MIN2S(120)) >= GetTickCount64()/1000) {
+ if ((time + MIN2S(120)) >= (uint32) time(NULL)) { // wxCas/wxcasframe.cpp
- ( long ) ( ts.tv_sec ) ) ) ); // Stored in Ticks
+ ( long ) ( time(NULL) ) ) ) );Five sites total, all single-line. Original PR keeps its Y2038 win and gains the clock-skew immunity for free. 2. Cosmetic only: |
|
Small clarification on the wxCas hunk — with the @@ WxCasFrame::WxCasFrame ( const wxString & title ) :
m_maxLineCount = 0;
- struct timespec ts;
- clock_gettime(CLOCK_REALTIME, &ts);
-
// Check if we have a previous DL max hit
double absoluteMaxDL = ( double ) ( prefs->Read ( WxCasCte::ABSOLUTE_MAX_DL_KEY, 0L ) ) / 1024.0; // Stored in bytes
wxDateTime absoluteMaxDlDate( ( time_t ) ( prefs->Read ( WxCasCte::ABSOLUTE_MAX_DL_DATE_KEY,
- ( long ) ( ts.tv_sec ) ) ) ); // Stored in Ticks
+ ( long ) ( time(NULL) ) ) ) ); |
integrated suggestion by @got3nks to use CLOCK_MONOTONIC instead of CLOCK_REALTIME, to be safe in case of negative jump because of NTP or user adjusted wall-clock going backwards
|
Thanks for your review @got3nks I integrated your suggestions and pushed a new commit. Indeed, most of the calls are MONOTONIC territory, let's make the full fix since we are here. I just needed to slightly adjust the second PartFile.cpp diff, since there was a variable called "time" that I needed to rename before calling time(NULL). Compiled and running on my side, I will let it run a couple of hours and propose to merge if it goes fine. |
3 participants
Get aMule ready for Y2038
Fixes #602
Introduction
This pull request aims to fix several issues concerning time management in aMule that arised when analyzing Y2038 compatibility of aMule.
There are several points:
GetTickCount.{h,cpp} files analysis
These files define the function
uint32 GetTickCount()used everywhere in the code to manage timeouts. For non-WINDOWS builds, this function callsGetTickCountFullRes()and returns the number of milliseconds from Jan 1 1970. However, the return type is uint32. If it were seconds, uint32 will last 136 years, but for milliseconds, it can hold only 2^32 milliseconds = 50 days aprox. This means this function has been overflowing every ~50 days after Jan 1 1970.I wrote a small program to tell me the previous and the next "50-days timeout reset" event:
Output:
When running aMule on that moment, we can check on the Verbose Debug log that multiple errors are triggered. Also, aMule freezes for some seconds, probably from other timeouts triggering events like writing files, drawing windows, etc... Apparently no big deal, but not pretty and error-prone, and this uint32 storing milliseconds could hide other problems. It is worth fixing. Example of log output when simulating that date:
Also, this function uses the deprecated
gettimeofday()to retrieve time, which shall be replaced byclock_gettime(). There is also aGetTickCount64()function which returns uint64, but is much less used and also calls gettimeofday().Additionally, the file contains an unused
MyTimer class, which has been removed.Ironically, the WINDOWS function
GetTickCount_64()seems to be the only already free of issues for aMule.The fix consists in leaving only one function,
uint64 GetTickCount64(), using clock_gettime(). Then, replace all calls to the removed functions with this one, and use uint64 datatypes in all code calling it.On a final note concerning GetTickCount.{h,cpp}, it defines a global variable
uint32 TheTime. Since it counts seconds from application start-up, 32 bits are enough.As a bonus, I removed a couple of GetTickCount used to initialize variables that were never used (example: m_nCreationTime in BaseClient.cpp).
And also improved consecutive >=2 GetTickCount() calls in multiple places. Just do one call and store the return value. Example: change this
to this
Fix
Out of scope
Testing
I have been running this version on a VM for several days, with the time set to 2038-01-19, performed multiple searches, downloads, restart to read/write conf files, etc...
Opening this pull request as a Draft to collect some reviews / extra testing.