KSyCoca, Thread safety, and Cache invalidation

[David Faure]

On Monday 14 September 2015 20:49:26 Thiago Macieira wrote:
> On Sunday 13 September 2015 23:04:01 David Faure wrote:
> > On Tuesday 14 July 2015 16:01:09 Thiago Macieira wrote:
> > > If you need a machine-comparable time with other systems or across
> > > reboots,
> > > use QDateTime::currentDateTimeUtc(). That avoids refreshing the timezone
> > > database to convert to local time.
> > > 
> > > Only use QDateTime::currentDateTime() if you want to show something to the
> > > user. There's no other valid reason. (writing to a log counts as "showing
> > > to the user")
> > 
> > What if I need to compare a file's modification time with a given timestamp?
> 
> File times are kept in UTC on disk.

Wow, so we do quite some unnecessary roundtrips between UTC and localtime indeed.

> > This code:
> > 
> >     qCDebug(SYCOCA) << "checking file timestamps";
> >     const QDateTime stamp = QDateTime::fromMSecsSinceEpoch(timestamp);
> 
> You're using the function that creates a LocalTime timestamp and yet:

QDateTime::fromMSecsSinceEpoch() indeed creates a localtime timestamp,
which means it calls qt_localtime() which calls tzset(), so it's not threadsafe.

#1  0x00007ffff7118d1f in qt_tzset () at tools/qdatetime.cpp:2117
#2  0x00007ffff7119194 in qt_localtime (msecsSinceEpoch=1447506886000, localDate=0x7fffffffbed0, localTime=0x7fffffffbec0, daylightStatus=0x7fffffffbebc) at tools/qdatetime.cpp:2
333
#3  0x00007ffff7119622 in epochMSecsToLocalTime (msecs=1447506886000, localDate=0x7fffffffbed0, localTime=0x7fffffffbec0, daylightStatus=0x7fffffffbebc) at tools/qdatetime.cpp:24
40
#4  0x00007ffff711aecb in QDateTime::setMSecsSinceEpoch (this=0x7fffffffc110, msecs=1447506886000) at tools/qdatetime.cpp:3436
#5  0x00007ffff711c876 in QDateTime::fromMSecsSinceEpoch (msecs=1447506886000, spec=Qt::LocalTime, offsetSeconds=0) at tools/qdatetime.cpp:4267
#6  0x00007ffff711c77f in QDateTime::fromTime_t (seconds=1447506886) at tools/qdatetime.cpp:4187
#7  0x00007ffff71f529c in QFileSystemMetaData::modificationTime (this=0x62edf0) at io/qfilesystemmetadata_p.h:274
#8  0x00007ffff71f4f5c in QFileInfo::lastModified (this=0x7fffffffc198) at io/qfileinfo.cpp:1336

Who would expect this to not be threadsafe?

> > Should we have a QFileInfo::lastModifiedUtc()? Or to make this all much more
> > intuitive, should there be a mutex in epochMSecsToLocalTime and
> > localMSecsToEpochMSecs? [the same one, obviously]
> 
> What if we changed the existing lastModified() to return UTC? As I said, the 
> date is stored in UTC already.
> 
> Though I imagine quite a few places don't convert to local time and would 
> start showing UTC time unexpectedly...

Yeah, I'm 100% sure most code out there doesn't convert to localtime,
hence my suggestion of QFileInfo::lastModifiedUtc(), to avoid breaking code.

But then we also need QDateTime::fromMSecsSinceEpoch(time_t, UTC) ?

Is there really no way to do localtime/UTC conversions in a threadsafe way?
This tzset() issue is really awful, we'll never manage to make sure that 100%
of the code that needs to be threadsafe uses UTC everywhere.

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5