Obviously not for 0.14.
Though boost’s chrono hides the racy gmtime() issue nicely, it’s really just using gmtime_r internally. This switches to the same thing, but without the boost indirection.
PRing this separately from other boost removals because it’s not a 1:1 replacement.
Failed on both Windows builds with:
0utiltime.cpp: In function ‘const tm gmtime_int(time_t)’:
1utiltime.cpp:31:25: error: cannot convert ‘time_t* {aka long int*}’ to ‘tm*’ for argument ‘1’ to ‘errno_t gmtime_s(tm*, const time_t*)’
2 gmtime_s(&time, &out);
Unfortunately, there's still no standard way of printing the current time in a
threadsafe way. Digging down into boost's approach, they simply use gmtime_r
when possible, as guessed by availability macros.
We now use the same approach, but use autotools to detect whether gmtime_r or
gmtime_s can be used, or as a fallback, the racy gmtime.
Note that MilliSleep was not replaced because it is an interruption point. That
can be done once boost threads are all gone.
std::chrono::system_clock.time_since_epoch and time_t(0) are not guaranteed
to use the Unix epoch timestamp, but in practice they almost certainly will.
Any differing behavior will be assumed to be an error, unless certain
platforms prove to consistently deviate, at which point we'll cope with it
by adding offsets.
Do a quick runtime check to verify that
time_t(0) == std::chrono::system_clock's epoch time == unix epoch.
Of course windows has reversed arguments. Why wouldn’t it?
Pushed the reversal.
38+ out = *gmtime(&time);
39+#endif
40+ return out;
41+}
42+
43+bool ChronoSanityCheck()
Needing something like this makes me wonder if we’re not better off just using the C time functions, which guarantee being based on the UNIX epoch.
Especially as we’re already probing for gmtime_int anyway. This seems double.
Do we need something that is only offered by std::chrono? (or needs much less code with std::chrono)
@laanwj Best I can tell from google and reading the c spec, it’s not guaranteed to be based on the UNIX epoch either :(. Of course, I’ve never seen an implementation that isn’t, as that would likely break years worth of assumptions. And based on that, I would assume that c++ implementations will be using the underlying c implementation.
So my logic was: if we can run a quick sanity check here to be sure that c/c++/unix epoch are all aligned, then we can use c apis and std::chrono interchangeably throughout the codebase without worry.
As for why chrono here, it was to avoid gettimeofday portability issues. If you’d prefer to work around those instead, no problem.
You’re right though that DateTimeStrFormat is kinda clunky. I only used c++ there to avoid c string size guessing. I’m also happy to switch that if you’d prefer.
Best I can tell from google and reading the c spec, it’s not guaranteed to be based on the UNIX epoch either :(
Which ones? The man page of “gmtime” tells me the following:
0 The ctime(), gmtime() and localtime() functions all take an argument of data type time_t, which represents calendar time. When interpreted as an absolute time value, it represents the
1 number of seconds elapsed since the Epoch, 1970-01-01 00:00:00 +0000 (UTC).
Same for gettimeofday:
0 and gives the number of seconds and microseconds since the Epoch (see time(2)). The tz argument is a struct timezone:
If using std::chrono makes anything less hassle I’m fine with using it. It just seems like a lot of code necessary to work around issues (and that’s just to get started using it!), but you’re right that gettimeofday and strftime have their own issues.
@laanwj Posix uses a value since the Unix epoch, but time_t is implementation-defined according to the c spec. But it really doesn’t matter, realistically it’ll be the Unix epoch everywhere we run. Sorry for the distraction.
I only brought it up because the c++ system_clock’s epoch is technically implementation-defined as well, I was just making the case that it’s probably just as safe to assume as with c.
PING and PONG, the int64 for node latency can overflow. Without much review, does this change any of the behavior in that respect? chrono::steady_clock assures monotone time where required.
@TheBlueMatt This is intended to just be a replacement of what we have now. The behavioral changes can come next.
I have an impl of a clock/time_point that I’ve made that is explicitly non-convertable from one type to another. Happy to PR that for discussion as well. I expect that it would replace our current timestamp usage, but I figured it’d be best to get rid of boost first, since the boost condvars use boost::chrono.
60+ if (nTime != zeroTime)
61+ return false;
62+
63+ // Check that the above zero time is actually equal to the known unix timestamp.
64+ tm epoch = gmtime_int(nTime);
65+ if ((epoch.tm_sec != 0) || \
\ at the end of the line?
51+ // Create a new clock from time_t(0) and make sure that it represents 0
52+ // seconds from the system_clock's time_since_epoch. Then convert that back
53+ // to a time_t and verify that it's the same as before.
54+ const time_t zeroTime{};
55+ auto clock = std::chrono::system_clock::from_time_t(zeroTime);
56+ if (std::chrono::duration_cast<std::chrono::seconds>(clock.time_since_epoch()).count() != 0)
Labels
Refactoring
Milestone
0.17.0