fix: parsing end dates on systems with 32-bit time_t #24
Conversation
greghuels
force-pushed
the
greg.huels/FFESUPPORT-348/fix-end-date-parsing
branch
from
9eccf3f to
934a512
Compare
src/time_utils.cpp
Outdated
| // Special handling for year 9999: use max representable time_point | ||
| // Year 9999 is commonly used to represent "no end date" but may not fit in time_t on all | ||
| // systems | ||
| if (tm.tm_year == 9999 - 1900) { | ||
| return std::chrono::system_clock::time_point::max(); | ||
| } |
There was a problem hiding this comment.
major: this feels super-arbitrary. Someone may use 8000 instead and that wouldn't fit either.
There was a problem hiding this comment.
@dd-oleksii I pushed a different approach. Let me know what you think.
greghuels
force-pushed
the
greg.huels/FFESUPPORT-348/fix-end-date-parsing
branch
from
3569a9e to
4e9a2b1
Compare
greghuels
force-pushed
the
greg.huels/FFESUPPORT-348/fix-end-date-parsing
branch
from
2419122 to
d114b0c
Compare
greghuels
force-pushed
the
greg.huels/FFESUPPORT-348/fix-end-date-parsing
branch
from
d114b0c to
ed495e8
Compare
| // Handle gmtime failure (can happen on 32-bit systems with out-of-range dates) | ||
| if (!tm_ptr) { | ||
| return "1970-01-01T00:00:00.000Z"; // Return epoch as fallback | ||
| } |
There was a problem hiding this comment.
Epoch is different on Posix vs Windows. It could be fine to use the Posix epoch since it's unlikely that clients will need to represents dates outside of it, but in general maybe it'd be better to not return any string if tp doesn't represent a valid point in time?
Otherwise, with out-of-range dates, was the caller intending to represent a date in the far future, or far past? This formatting function is used to set "updatedAt" in BanditConfiguration. Setting it as epoch means this will always be the "oldest" bandit config possible, is this correct?
| // Check for the sentinel value that represents "no end date" | ||
| if (timestamp == "9999-12-31T00:00:00.000Z") { | ||
| return std::chrono::system_clock::time_point::max(); | ||
| } | ||
| return parseISOTimestamp(timestamp, std::chrono::system_clock::time_point::max()); |
There was a problem hiding this comment.
this still feels arbitrary, I'll echo the concern @dd-oleksii had. What if someone sets (explicitly) that an assignment ends in the year 5000? Or, more realistically, 2039.
I think it might be better to make sure parseISOTimestamp is overflow-safe on its own, and "naturally" saturates to time_point::max() if timestamp is bigger than you can represent in a time_point on this platform. And I think your changes already (almost) do it, so no need for this check.
The platform I've tested on that had the problem was Windows 11 x64:
std::get_timeworks as expected for 9999 and returns a validstd::tmwith tm_year 8099, tm_mon 11, tm_mday 31time_c = _mkgmtime(&tm);returns -1 even though std::time_t is 64 bit! Same with_mkgmtime64- after your change with
return errorValue;it should be enough to handle this case - the one thing that can still go wrong is overflow in
tp += std::chrono::milliseconds(milliseconds);, iftpis almost on max already. Maybe skip the whole millisecond parsing? 😄
Also, I have another idea:
- change return type of
parseISOTimestamptooptional<time_point>and return a nullopt for when thetimestampcannot be represented on the platform - this makes sense because the callsite is
a.endAt = parseISOTimestampanda.endAtis already an optional - and the code that matches assignments, shards etc. skips the date check when the timestamp is nullopt
- behaviorally, this ends up working the same as returning "max" or "min" depending on call context, but without needing
parseISOTimestampForConfigStartTimeandparseISOTimestampForConfigEndTime
There was a problem hiding this comment.
Hah, I found this:
The range of the _mkgmtime32 function is from midnight, January 1, 1970, UTC to 23:59:59 January 18, 2038, UTC. The range of _mkgmtime64 is from midnight, January 1, 1970, UTC to 23:59:59, December 31, 3000, UTC. An out-of-range date results in a return value of -1.
I expected the 64 bit version would have astronomical upper time limit, because why not, but 9999 really is out of range on Windows 🙃
There was a problem hiding this comment.
I think it might be better to make sure parseISOTimestamp is overflow-safe on its own, and "naturally" saturates to time_point::max() if timestamp is bigger than you can represent in a time_point on this platform. And I think your changes already (almost) do it, so no need for this check.
Yeah, this is a better approach. I'll update!
| // Special handling for max time_point (year 9999 dates) | ||
| if (tp == std::chrono::system_clock::time_point::max()) { | ||
| return "9999-12-31T00:00:00.000Z"; | ||
| } |
There was a problem hiding this comment.
Q: does this date stay on the client, or goes back to the backend (for consumption by potentially other clients with different date parsing capabilities)?
If it stays on the client, we could just let formatISOTimestamp do its best at representing the largest date it can - it should get parsed back to max() in parseISOTimestamp, if the implementations are symmetrical. In that case, no need for this special-casing.
There was a problem hiding this comment.
It does stay on the client. That's a good idea.
| - name: Test with 32-bit time_t | ||
| run: | | ||
| docker run --rm --platform linux/386 -v $PWD:/workspace -w /workspace debian:bookworm bash -c " |
There was a problem hiding this comment.
Not sure if this represents my case, this might be a windows vs linux problem rather than 32 vs 64. I've had this problem with x64 Windows that has 64-bit time_t - the problem is caused by the use _mkgmtime. Would be good to test on windows too, but I understand it could be a logistical problem
|
Closing in favor of #35 |
Eppo Internal:
🎟️ Fixes FFESUPPORT-348
See also: #22
Motivation and Context
When parsing the configuration value
"endAt": "9999-12-31T00:00:00.000Z", the SDK was returning{__d_={__rep_=0}}(epoch/zero) instead of a far-future date on systems with 32-bittime_t. This causes all allocations with year 9999 end dates to be rejected withAFTER_END_TIMEerror code.