Date and time classes not thread safe

[Idar Tollefsen]

Hello,

time(), gettimeofday() and possibly localtime() are not thread safe functions. 
time() is, on some platforms at least, implemented by using gettimeofday().

The problem, according to POSIX documentation, is that time(), and therefore 
gettimeofday(), keeps some static data that is overwritten by each call to the 
function. Needless to say, this caused some havoc when several threads in the 
same address space tried to use both the mentioned time functions and the 
ost::Time, ost::Date and ost::Datetime classes.

I know some platforms have reentrant versions of the time functions with a _r 
suffix to the function name, but they aren't available on all platforms.

My suggested solution is to introduce a ost::SysTime class that has a static 
semaphore and static functions that replaces the standard, non-thread safe, time 
functions, much like ost::Thread::sleep() does for the standard sleep() 
function. The functions in ost::SysTime then locks down access to the standard 
functions with the help of the semaphore.

I have attached a patch that introduces this class and replaces all uses of 
time(), gettimeofday() and localtime() internally in CommonCpp2 with the 
equivalent function from ost::SysTime. The patch applies cleanly to a CVS 
checkout of commoncpp2 as of today, May 13th 14:15 GMT+1.

I've used this patch for about a week now, and also replaced all calls to the 
standard time functions in our project with those introduced with ost::SysTime. 
This has solved all threading problems related to the use of time functions.

Now, I'm not saying the attached solution is ideal. I would imagine it requires 
some more thought as to the function names and overall strategy. It does however 
highlight the problem and point out a possible solution. The patch also contains 
my fixes for ost::Time::getTime() and ThreadImpl::ThreadDestructor() that I sent 
in earlier as they hadn't been fixed in the CVS version (but really do cause 
problems if not fixed).

The patch hasn't been tested on Win32 _at all_. I don't have access to a Windows 
machine with a development environment set up. It has seen most testing on OS X, 
some on FreeBSD and has been compiled on Linux.

I can provide patches for the individual files instead if desired, just let me 
know.


Sincerely,
Idar Tollefsen

--- commoncpp2.old/include/cc++/numbers.h       Sun Oct 12 00:34:59 2003
+++ commoncpp2/include/cc++/numbers.h   Tue May 11 11:44:42 2004
@@ -58,6 +58,10 @@
 #include <cc++/string.h>
 #endif
 
+#ifndef        CCXX_THREAD_H_
+#include <cc++/thread.h>
+#endif
+
 #include <ctime>
 
 #ifdef CCXX_NAMESPACES
@@ -299,6 +303,58 @@
 
        String strftime(const char *format) const;
 };
+
+/**
+ * This class is used to access non-reentrant date and time functions in the
+ * standard C library.
+ *
+ * The class has two purposes:
+ * - 1 To be used internaly in CommonCpp's date and time classes to make them
+ *     thread safe.
+ * - 2 To be used by clients as thread safe replacements to the standard C
+ *     functions, much like Thread::sleep() represents a thread safe version
+ *     of the standard sleep() function.
+ *
+ * @note The class provides one function with the same name as its equivalent
+ *       standard function and one with another, unique name. For new clients,
+ *       the version with the unique name is recommended to make it easy to
+ *       grep for accidental usage of the standard functions. The version with
+ *       the standard name is provided for existing clients to sed replace 
their
+ *       original version.
+ *
+ * @note Also note that some functions that returned pointers have been redone
+ *       to take that pointer as an argument instead, making the caller
+ *       responsible for memory allocation/deallocation. This is almost
+ *       how POSIX specifies *_r functions (reentrant versions of the
+ *       standard time functions), except the POSIX functions also return the
+ *       given pointer while we do not. We don't use the *_r functions as they
+ *       aren't all generally available on all platforms yet.
+ *
+ * @author Idar Tollefsen <address@hidden>
+ * @short Thread safe date and time functions.
+ */
+class __EXPORT SysTime
+{
+public:
+        static time_t getTime(time_t *tloc = NULL);
+        static time_t time(time_t *tloc) 
+                { return getTime(tloc); };
+
+        static int getTimeOfDay(struct timeval *tp);
+        static int gettimeofday(struct timeval *tp, struct timezone *tzp)
+                { return getTimeOfDay(tp); };
+        
+        static void getLocalTime(const time_t *clock, struct tm *result);
+        static void locatime(const time_t *clock, struct tm *result)
+                { getLocalTime(clock, result); };
+        
+protected:
+        static void lock();
+        static void unlock();
+
+private:
+        static Semaphore timeLock;
+}; 
 
 /**
  * A number class that manipulates a string buffer that is also a date.
--- commoncpp2.old/src/date.cpp Thu May 13 13:57:41 2004
+++ commoncpp2/src/date.cpp     Thu May 13 14:00:13 2004
@@ -39,8 +39,6 @@
 // If you do not wish that, delete this exception notice.
 
 #include <cc++/config.h>
-#include <cc++/thread.h>
-#include <cc++/string.h>
 #include <cc++/exception.h>
 #include <cc++/export.h>
 #include <cc++/numbers.h>
@@ -64,14 +62,15 @@
 #endif
 #endif
 
+Semaphore SysTime::timeLock(1);
+
 Date::Date()
 {
-       time_t now;
-       struct tm *dt;
-       time(&now);
-       dt = localtime(&now);
+       time_t now = SysTime::getTime();
+       struct tm dt;
+       SysTime::getLocalTime(&now, &dt);
 
-       toJulian(dt->tm_year + 1900, dt->tm_mon + 1, dt->tm_mday);
+       toJulian(dt.tm_year + 1900, dt.tm_mon + 1, dt.tm_mday);
 }
 
 Date::Date(struct tm *dt)
@@ -81,8 +80,9 @@
 
 Date::Date(time_t tm)
 {
-       struct tm *dt = localtime(&tm);
-       toJulian(dt->tm_year + 1900, dt->tm_mon + 1, dt->tm_mday);
+       struct tm dt;
+       SysTime::getLocalTime(&tm, &dt);
+       toJulian(dt.tm_year + 1900, dt.tm_mon + 1, dt.tm_mday);
 }
 
 Date::Date(char *str, size_t size)
@@ -92,8 +92,9 @@
 
 void Date::setDate(const char *str, size_t size)
 {
-       time_t now;
-       struct tm *dt;
+       time_t now = SysTime::getTime();
+       struct tm dt;
+       SysTime::getLocalTime(&now, &dt);
        int year = 0;
        const char *mstr = str;
        const char *dstr = str;
@@ -106,9 +107,7 @@
 #ifdef DEBUG
                cout << "Date::SetDate(): 0000" << endl;
 #endif
-               time(&now);
-               dt = localtime(&now);
-               year = dt->tm_year + 1900;
+               year = dt.tm_year + 1900;
                mstr = str;
                dstr = str + 2;
        }               
@@ -118,9 +117,7 @@
 #ifdef DEBUG
                cout << "Date::SetDate(): 00/00" << endl;
 #endif
-               time(&now);
-               dt = localtime(&now);
-               year = dt->tm_year + 1900;
+               year = dt.tm_year + 1900;
                mstr = str;
                dstr = str + 3;
        }
@@ -130,10 +127,8 @@
 #ifdef DEBUG
                cout << "Date::SetDate(): 000000" << endl;
 #endif
-               time(&now);
-               dt = localtime(&now);
                ZNumber nyear((char*)str, 2);
-               year = ((dt->tm_year + 1900) / 100) * 100 + nyear();
+               year = ((dt.tm_year + 1900) / 100) * 100 + nyear();
                mstr = str + 2;
                dstr = str + 4;
        }
@@ -154,10 +149,8 @@
 #ifdef DEBUG
                cout << "Date::SetDate(): 00/00/00" << endl;
 #endif
-               time(&now);
-               dt = localtime(&now);
                ZNumber nyear((char*)str, 2);
-               year = ((dt->tm_year + 1900) / 100) * 100 + nyear();
+               year = ((dt.tm_year + 1900) / 100) * 100 + nyear();
                mstr = str + 3;
                dstr = str + 6;
        }
@@ -422,12 +415,11 @@
 
 Time::Time()
 {
-       time_t now;
-       struct tm *dt;
-       time(&now);
-       dt = localtime(&now);
+       time_t now = SysTime::getTime();
+       struct tm dt;
+       SysTime::getLocalTime(&now, &dt);
 
-       toSeconds(dt->tm_hour, dt->tm_min, dt->tm_sec);
+       toSeconds(dt.tm_hour, dt.tm_min, dt.tm_sec);
 }
 
 Time::Time(struct tm *dt)
@@ -437,8 +429,9 @@
 
 Time::Time(time_t tm)
 {
-       struct tm *dt = localtime(&tm);
-       toSeconds(dt->tm_hour, dt->tm_min, dt->tm_sec);
+       struct tm dt;
+       SysTime::getLocalTime(&tm, &dt);
+       toSeconds(dt.tm_hour, dt.tm_min, dt.tm_sec);
 }
 
 Time::Time(char *str, size_t size)
@@ -466,18 +459,7 @@
 
 time_t Time::getTime(void) const
 {
-       char buf[7];
-       struct tm dt;
-       memset(&dt, 0, sizeof(dt));
-       fromSeconds(buf);
-       ZNumber nhour(buf, 2);
-       ZNumber nminute(buf + 2, 2);
-       ZNumber nsecond(buf + 4, 2);
-       
-       dt.tm_hour = nhour();
-       dt.tm_min = nminute();
-       dt.tm_sec = nsecond();
-       return mktime(&dt);
+       return static_cast<time_t>(seconds);
 }
 
 int Time::getHour(void) const
@@ -544,9 +526,9 @@
        char buf[7];
 
        fromSeconds(buf);
-       String time(buf);
+       String strTime(buf);
 
-       return time;
+       return strTime;
 }
 
 long Time::getValue(void) const
@@ -691,9 +673,10 @@
 
 Datetime::Datetime(time_t tm)
 {
-       struct tm *dt = localtime(&tm);
-       toJulian(dt->tm_year + 1900, dt->tm_mon + 1, dt->tm_mday);
-       toSeconds(dt->tm_hour, dt->tm_min, dt->tm_sec);
+       struct tm dt;
+       SysTime::getLocalTime(&tm, &dt);
+       toJulian(dt.tm_year + 1900, dt.tm_mon + 1, dt.tm_mday);
+       toSeconds(dt.tm_hour, dt.tm_min, dt.tm_sec);
 }
 
 Datetime::Datetime(tm *dt) :
@@ -752,13 +735,12 @@
 
 Datetime::Datetime() : Date(), Time()
 {
-       time_t now;
-       struct tm *dt;
-       time(&now);
-       dt = localtime(&now);
+       time_t now = SysTime::getTime();
+       struct tm dt;
+       SysTime::getLocalTime(&now, &dt);
 
-       toSeconds(dt->tm_hour, dt->tm_min, dt->tm_sec);
-       toJulian(dt->tm_year + 1900, dt->tm_mon + 1, dt->tm_mday);
+       toSeconds(dt.tm_hour, dt.tm_min, dt.tm_sec);
+       toJulian(dt.tm_year + 1900, dt.tm_mon + 1, dt.tm_mday);
 }
 
 bool Datetime::isValid(void) const
@@ -900,21 +882,82 @@
        char buffer[64];
        int last;
        time_t t;
-       tm *tbp;
+       tm tbp;
        String retval;
 
        t = getDatetime();
-        tbp = localtime(&t);
+       SysTime::getLocalTime(&t, &tbp);
 #ifdef WIN32
-       last = ::strftime(buffer, 64, format, tbp);
+       last = ::strftime(buffer, 64, format, &tbp);
 #else
-       last = std::strftime(buffer, 64, format, tbp);
+       last = std::strftime(buffer, 64, format, &tbp);
 #endif
 
        buffer[last] = '\0';
        retval = buffer;
        return retval;
+}
+
+time_t SysTime::getTime(time_t *tloc) 
+{
+    time_t ret;
+    lock();
+    time_t temp;
+#ifdef WIN32
+    ::time(&temp);
+#else
+    std::time(&temp);
+#endif
+    memcpy(&ret, &temp, sizeof(time_t));
+    if (tloc != NULL) 
+        memcpy(tloc, &ret, sizeof(time_t));
+    unlock();
+    return ret;   
+}
+
+int SysTime::getTimeOfDay(struct timeval *tp) 
+{
+    int ret(0);
+    lock();
+
+#ifdef WIN32
+    // We could use _ftime(), but it is not available on WinCE.
+    // (WinCE also lacks time.h)
+    // Note also that the average error of _ftime is around 20 ms :)
+    DWORD ms = GetTickCount();
+    tv_->tv_sec = ms / 1000;
+    tv_->tv_usec = ms * 1000;
+#else
+    struct timeval temp;
+    ret = ::gettimeofday(&temp, NULL);
+    if (ret == 0) 
+        memcpy(tp, &temp, sizeof(struct timeval));
+#endif
+
+    unlock();
+    return ret;
+}
 
+void SysTime::getLocalTime(const time_t *clock, struct tm* result) 
+{
+    lock();
+#ifdef WIN32
+    struct tm *temp = ::localtime(clock);
+#else
+    struct tm *temp = std::localtime(clock);
+#endif
+    memcpy(result, temp, sizeof(struct tm));
+    unlock();
+}
+
+void SysTime::lock()
+{
+    timeLock.wait();
+}
+
+void SysTime::unlock() 
+{
+    timeLock.post();
 }
 
 DateNumber::DateNumber(char *str) :
--- commoncpp2.old/src/friends.cpp      Tue May 11 11:57:31 2004
+++ commoncpp2/src/friends.cpp  Tue May 11 12:38:56 2004
@@ -41,6 +41,7 @@
 #include <cc++/config.h>
 #include <cc++/export.h>
 #include <cc++/thread.h>
+#include <cc++/numbers.h>
 #include "private.h"
 
 #include <cstdlib>
@@ -81,7 +82,7 @@
 #else
        struct timeval current;
 
-       gettimeofday(&current, NULL);
+       SysTime::getTimeOfDay(&current);
        spec->tv_sec = current.tv_sec + timer / 1000;
        spec->tv_nsec = (current.tv_usec + (timer % 1000) * 1000) * 1000;
 #endif
--- commoncpp2.old/src/port.cpp Tue May 11 11:57:31 2004
+++ commoncpp2/src/port.cpp     Tue May 11 12:40:58 2004
@@ -41,6 +41,7 @@
 #include <cc++/config.h>
 #include <cc++/export.h>
 #include <cc++/socket.h>
+#include <cc++/numbers.h>
 #include "private.h"
 #ifndef WIN32
 #include <cerrno>
@@ -57,12 +58,12 @@
 TimerPort::TimerPort()
 {
        active = false;
-       gettimeofday(&timer, NULL);
+       SysTime::getTimeOfDay(&timer);
 }
 
 void TimerPort::setTimer(timeout_t timeout)
 {
-       gettimeofday(&timer, NULL);
+       SysTime::getTimeOfDay(&timer);
        active = false;
        if(timeout)
                incTimer(timeout);
@@ -96,7 +97,7 @@
        if(!active)
                return TIMEOUT_INF;
 
-       gettimeofday(&now, NULL);
+       SysTime::getTimeOfDay(&now);
        diff = (timer.tv_sec - now.tv_sec) * 1000l;
        diff += (timer.tv_usec - now.tv_usec) / 1000l;
 
@@ -114,7 +115,7 @@
        if(!active)
                return TIMEOUT_INF;
 
-       gettimeofday(&now, NULL);
+       SysTime::getTimeOfDay(&now);
        diff = (now.tv_sec -timer.tv_sec) * 1000l;
        diff += (now.tv_usec - timer.tv_usec) / 1000l;
        if(diff < 0)
--- commoncpp2.old/src/thread.cpp       Tue May 11 11:57:31 2004
+++ commoncpp2/src/thread.cpp   Tue May 11 12:41:48 2004
@@ -42,6 +42,7 @@
 #include <cc++/export.h>
 #include <cc++/thread.h>
 #include <cc++/exception.h>
+#include <cc++/numbers.h>
 #include <cstdio>
 #include "private.h"
 
@@ -908,7 +909,7 @@
 // delete Thread class created for no CommonC++ thread
 inline void ThreadImpl::ThreadDestructor(Thread* th)
 {
-       if (!th || th == DUMMY_INVALID_THREAD)
+       if (!th || th == DUMMY_INVALID_THREAD || !th->priv)
                return;
        if (th->priv->_type == threadTypeDummy)
                delete th;
@@ -1296,7 +1297,7 @@
 PosixThread::PosixThread(int pri, size_t stack):
        Thread(pri,stack)
 {
-       time(&_alarm);
+       SysTime::getTime(&_alarm);
 }
 
 void PosixThread::setTimer(timeout_t timer, bool periodic)
@@ -1321,7 +1322,7 @@
        _arm.enterMutex();
        _timer = this;
 #endif
-       time(&_alarm);
+       SysTime::getTime(&_alarm);
        sigemptyset(&sigs);
        sigaddset(&sigs, SIGALRM);
        pthread_sigmask(SIG_UNBLOCK, &sigs, NULL);
@@ -1346,8 +1347,7 @@
        return (timeout_t)(itimer.it_value.tv_sec * 1000 +
                itimer.it_value.tv_usec / 1000);
 #else
-       time_t now;
-       time(&now);
+       time_t now = SysTime::getTime();
        return (timeout_t)(((now - _alarm) * 1000) + 500);
 #endif
 }