Throw a runtime error instead of terminating

[kalliste]

Elsewhere in this file if something goes wrong we throw a runtime error. If we also do that here instead of calling std::terminate we can catch it and fix the calling code.

[HowardHinnant]

This is very difficult logic.

Have you ever had a case where this terminate was called? If so, can you reproduce it? How do you propose that the calling code do when this exception is caught? Or if uncaught, what can the programmer change when discovering the uncaught exception?

[kalliste]

I had a case where terminate was called. It has happened multiple times for end users of my program but I haven't reproduced it yet. I may be able to.

I think probably the cause was that I was calling make_zoned with either NULL or a string that was not a valid timezone in this code:

struct tm t = pAlert->m_TimeStart;                                                                                                                                                                                                                                                                                            
auto system = sys_days{year{t.tm_year + 1900}/(t.tm_mon+1)/t.tm_mday} + hours{t.tm_hour} + minutes{t.tm_min} + seconds{t.tm_sec};
auto local = make_zoned(szCtlrTimezone, system);
auto lt = local.get_local_time();
auto ld = floor<days>(lt);
time_of_day<seconds> tod{lt - ld};                  
year_month_day ymd{ld};

switch (iIntID)
{
    case SF_INT_ID2_NOTIFICATION_YEAR_LOCAL:          return int{ymd.year()};
    case SF_INT_ID2_NOTIFICATION_MONTH_LOCAL:         return unsigned{ymd.month()};
    case SF_INT_ID2_NOTIFICATION_DAY_LOCAL:           return unsigned{ymd.day()};
    case SF_INT_ID2_NOTIFICATION_HOUR_LOCAL:          return tod.hours().count();
    case SF_INT_ID2_NOTIFICATION_MINUTE_LOCAL:        return tod.minutes().count();
}

Currently I am wrapping this whole section in a try/catch so if it throws instead of terminates then I can log what I was doing when I screwed up

[HowardHinnant]

If you pass in a bad time zone name, you'll get an exception with a .what() that says something like: "bad_name not found in timezone database", where "bad_name" is the name you passed in.

So NULL sounds like a more likely culprit. I get a segfault with that. But it didn't make it to the terminate() you propose changing. But guarding against that NULL sounds reasonable.

[HowardHinnant]

I've added checks to the zoned_time constructors to check for nullptr and throw if found: https://github.com/HowardHinnant/date/commit/d544e5af2536bf5319367cb7dafe6928d4205cd2

[dkhalanskyjb]

I did encounter an assertion error when trying to query the TZDB for dates earlier than the minimum year.

[HowardHinnant]

If you've got a test case I'd be happy to explore it. I did this and got garbage, but no assert:

#include "date/tz.h"
#include <iostream>

int
main()
{
    using namespace date;
    using namespace std;
    using namespace std::chrono;

    zoned_time zt{current_zone(), sys_days{} - years{50'000}};
    auto lt = zt.get_local_time();
    cout << lt << '\n';
}

[dkhalanskyjb]

Sure. With the current master,

current_zone()->get_info(local_seconds(seconds(-4266666666664)));

crashes for me with

src/tz.cpp:2128: date::sys_info date::time_zone::load_sys_info(std::vector<date::detail::transition>::const_iterator) const: Assertion `i != transitions_.begin()' failed.

I encountered this on both Linux and Mac.

On Linux, I compile the reproducer with these flags:

-lpthread -std=c++17 -DUSE_OS_TZDB=1

[HowardHinnant]

Thanks much @dkhalanskyjb ! Fixed with https://github.com/HowardHinnant/date/commit/658a3b94955d876675caca033fb799f5169a389c