Merge pull request #2715 from sigiesec/add-timers-tests

Problem: insufficient tests for zmq_timers_* and inadequate behaviour in corner cases

src/timers.cpp

@@ -31,11 +31,10 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "timers.hpp"
 #include "err.hpp"
 
-zmq::timers_t::timers_t () :
-tag (0xCAFEDADA),
-next_timer_id (0)
-{
+#include <algorithm>
 
+zmq::timers_t::timers_t () : tag (0xCAFEDADA), next_timer_id (0)
+{
 }
 
 zmq::timers_t::~timers_t ()
@@ -49,20 +48,45 @@ bool zmq::timers_t::check_tag ()
     return tag == 0xCAFEDADA;
 }
 
-int zmq::timers_t::add (size_t interval_, timers_timer_fn handler_, void* arg_)
+int zmq::timers_t::add (size_t interval_, timers_timer_fn handler_, void *arg_)
 {
-    uint64_t when = clock.now_ms() + interval_;
+    if (!handler_) {
+        errno = EFAULT;
+        return -1;
+    }
+
+    uint64_t when = clock.now_ms () + interval_;
     timer_t timer = {++next_timer_id, interval_, handler_, arg_};
     timers.insert (timersmap_t::value_type (when, timer));
 
     return timer.timer_id;
 }
 
+struct zmq::timers_t::match_by_id
+{
+    match_by_id (int timer_id_) : timer_id (timer_id_) {}
+
+    bool operator() (timersmap_t::value_type const &entry) const
+    {
+        return entry.second.timer_id == timer_id;
+    }
+
+  private:
+    int timer_id;
+};
+
 int zmq::timers_t::cancel (int timer_id_)
 {
-    cancelled_timers_t::iterator it = cancelled_timers.find (timer_id_);
+    // check first if timer exists at all
+    if (timers.end ()
+        == std::find_if (timers.begin (), timers.end (),
+                         match_by_id (timer_id_))) {
+        errno = EINVAL;
+        return -1;
+    }
 
-    if (it != cancelled_timers.end ()) {
+    // check if timer was already canceled
+    if (cancelled_timers.count (timer_id_)) {
         errno = EINVAL;
         return -1;
     }
@@ -74,32 +98,35 @@ int zmq::timers_t::cancel (int timer_id_)
 
 int zmq::timers_t::set_interval (int timer_id_, size_t interval_)
 {
-    for (timersmap_t::iterator it = timers.begin (); it != timers.end (); ++it) {
-        if (it->second.timer_id == timer_id_) {
-            timer_t timer = it->second;
-            timer.interval = interval_;
-            uint64_t when = clock.now_ms() + interval_;
-            timers.erase (it);
-                timers.insert (timersmap_t::value_type (when, timer));
+    const timersmap_t::iterator end = timers.end ();
+    const timersmap_t::iterator it =
+      std::find_if (timers.begin (), end, match_by_id (timer_id_));
+    if (it != end) {
+        timer_t timer = it->second;
+        timer.interval = interval_;
+        uint64_t when = clock.now_ms () + interval_;
+        timers.erase (it);
+        timers.insert (timersmap_t::value_type (when, timer));
 
-            return 0;
-        }
+        return 0;
     }
 
     errno = EINVAL;
     return -1;
 }
 
-int zmq::timers_t::reset (int timer_id_) {
-    for (timersmap_t::iterator it = timers.begin (); it != timers.end (); ++it) {
-        if (it->second.timer_id == timer_id_) {
-            timer_t timer = it->second;
-            uint64_t when = clock.now_ms() + timer.interval;
-            timers.erase (it);
-            timers.insert (timersmap_t::value_type (when, timer));
+int zmq::timers_t::reset (int timer_id_)
+{
+    const timersmap_t::iterator end = timers.end ();
+    const timersmap_t::iterator it =
+      std::find_if (timers.begin (), end, match_by_id (timer_id_));
+    if (it != end) {
+        timer_t timer = it->second;
+        uint64_t when = clock.now_ms () + timer.interval;
+        timers.erase (it);
+        timers.insert (timersmap_t::value_type (when, timer));
 
-            return 0;
-        }
+        return 0;
     }
 
     errno = EINVAL;
@@ -110,10 +137,11 @@ long zmq::timers_t::timeout ()
 {
     timersmap_t::iterator it = timers.begin ();
 
-    uint64_t now = clock.now_ms();
+    uint64_t now = clock.now_ms ();
 
     while (it != timers.end ()) {
-        cancelled_timers_t::iterator cancelled_it = cancelled_timers.find (it->second.timer_id);
+        cancelled_timers_t::iterator cancelled_it =
+          cancelled_timers.find (it->second.timer_id);
 
         //  Live timer, lets return the timeout
         if (cancelled_it == cancelled_timers.end ()) {
@@ -123,7 +151,7 @@ long zmq::timers_t::timeout ()
                 return 0;
         }
 
-        // Let's remove it from the begining of the list
+        // Let's remove it from the beginning of the list
         timersmap_t::iterator old = it;
         ++it;
         timers.erase (old);
@@ -138,10 +166,11 @@ int zmq::timers_t::execute ()
 {
     timersmap_t::iterator it = timers.begin ();
 
-    uint64_t now = clock.now_ms();
+    uint64_t now = clock.now_ms ();
 
     while (it != timers.end ()) {
-        cancelled_timers_t::iterator cancelled_it = cancelled_timers.find (it->second.timer_id);
+        cancelled_timers_t::iterator cancelled_it =
+          cancelled_timers.find (it->second.timer_id);
 
         //  Dead timer, lets remove it and continue
         if (cancelled_it != cancelled_timers.end ()) {
@@ -154,7 +183,7 @@ int zmq::timers_t::execute ()
 
         //  Map is ordered, if we have to wait for current timer we can stop.
         if (it->first > now)
-        break;
+            break;
 
         timer_t timer = it->second;
 

src/timers.hpp

@@ -102,6 +102,8 @@ namespace zmq
 
             timers_t (const timers_t&);
             const timers_t &operator = (const timers_t&);
+
+            struct match_by_id;
         };
     }
 

tests/test_timers.cpp

@@ -29,17 +29,6 @@
 
 #include "testutil.hpp"
 
-void sleep_ (long timeout_)
-{
-#if defined ZMQ_HAVE_WINDOWS
-    Sleep (timeout_ > 0 ? timeout_ : INFINITE);
-#elif defined ZMQ_HAVE_ANDROID
-    usleep (timeout_ * 1000);
-#else
-    usleep (timeout_ * 1000);
-#endif
-}
-
 void handler (int timer_id, void* arg)
 {
     (void) timer_id;               //  Stop 'unused' compiler warnings
@@ -52,13 +41,109 @@ int sleep_and_execute(void *timers_)
 
     //  Sleep methods are inaccurate, so we sleep in a loop until time arrived
     while (timeout > 0) {
-        sleep_ (timeout);
+        msleep (timeout);
         timeout = zmq_timers_timeout(timers_);
     }
 
     return zmq_timers_execute(timers_);
 }
 
+void test_null_timer_pointers ()
+{
+    void *timers = NULL;
+
+    int rc = zmq_timers_destroy (&timers);
+    assert (rc == -1 && errno == EFAULT);
+
+//  TODO this currently triggers an access violation
+#if 0
+  rc = zmq_timers_destroy (NULL);
+  assert (rc == -1 && errno == EFAULT);
+#endif
+
+    const size_t dummy_interval = 100;
+    const int dummy_timer_id = 1;
+
+    rc = zmq_timers_add (timers, dummy_interval, &handler, NULL);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_add (&timers, dummy_interval, &handler, NULL);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_cancel (timers, dummy_timer_id);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_cancel (&timers, dummy_timer_id);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_set_interval (timers, dummy_timer_id, dummy_interval);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_set_interval (&timers, dummy_timer_id, dummy_interval);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_reset (timers, dummy_timer_id);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_reset (&timers, dummy_timer_id);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_timeout (timers);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_timeout (&timers);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_execute (timers);
+    assert (rc == -1 && errno == EFAULT);
+
+    rc = zmq_timers_execute (&timers);
+    assert (rc == -1 && errno == EFAULT);
+}
+
+void test_corner_cases ()
+{
+    void *timers = zmq_timers_new ();
+    assert (timers);
+
+    const size_t dummy_interval = SIZE_MAX;
+    const int dummy_timer_id = 1;
+
+    //  attempt to cancel non-existent timer
+    int rc = zmq_timers_cancel (timers, dummy_timer_id);
+    assert (rc == -1 && errno == EINVAL);
+
+    //  attempt to set interval of non-existent timer
+    rc = zmq_timers_set_interval (timers, dummy_timer_id, dummy_interval);
+    assert (rc == -1 && errno == EINVAL);
+
+    //  attempt to reset non-existent timer
+    rc = zmq_timers_reset (timers, dummy_timer_id);
+    assert (rc == -1 && errno == EINVAL);
+
+    //  attempt to add NULL handler
+    rc = zmq_timers_add (timers, dummy_interval, NULL, NULL);
+    assert (rc == -1 && errno == EFAULT);
+
+    int timer_id = zmq_timers_add (timers, dummy_interval, handler, NULL);
+    assert (timer_id != -1);
+
+    //  attempt to cancel timer twice
+    //  TODO should this case really be an error? canceling twice could be allowed
+    rc = zmq_timers_cancel (timers, timer_id);
+    assert (rc == 0);
+
+    rc = zmq_timers_cancel (timers, timer_id);
+    assert (rc == -1 && errno == EINVAL);
+
+    //  timeout without any timers active
+    rc = zmq_timers_timeout(timers);
+    assert (rc == -1);
+
+    rc = zmq_timers_destroy (&timers);
+    assert (rc == 0);
+}
+
 int main (void)
 {
     setup_test_environment ();
@@ -77,7 +162,9 @@ int main (void)
     assert (!timer_invoked);
 
     //  Wait half the time and check again
-    sleep_ (zmq_timers_timeout (timers) / 2);
+    long timeout = zmq_timers_timeout(timers);
+    assert (rc != -1);
+    msleep (timeout / 2);
     rc = zmq_timers_execute (timers);
     assert (rc == 0);
     assert (!timer_invoked);
@@ -89,15 +176,17 @@ int main (void)
     timer_invoked = false;
 
     //  Wait half the time and check again
-    long timeout = zmq_timers_timeout (timers);
-    sleep_ (timeout / 2);
+    timeout = zmq_timers_timeout (timers);
+    assert (rc != -1);
+    msleep (timeout / 2);
     rc = zmq_timers_execute (timers);
     assert (rc == 0);
     assert (!timer_invoked);
 
     // Reset timer and wait half of the time left
     rc = zmq_timers_reset (timers, timer_id);
-    sleep_ (timeout / 2);
+    assert (rc == 0);
+    msleep (timeout / 2);
     rc = zmq_timers_execute (timers);
     assert (rc == 0);
     assert (!timer_invoked);
@@ -109,7 +198,8 @@ int main (void)
     timer_invoked = false;
 
     // reschedule
-    zmq_timers_set_interval (timers, timer_id, 50);
+    rc = zmq_timers_set_interval (timers, timer_id, 50);
+    assert (rc == 0);
     rc = sleep_and_execute(timers);
     assert (rc == 0);
     assert (timer_invoked);
@@ -117,8 +207,10 @@ int main (void)
 
     // cancel timer
     timeout = zmq_timers_timeout (timers);
-    zmq_timers_cancel (timers, timer_id);
-    sleep_ (timeout * 2);
+    assert (rc != -1);
+    rc = zmq_timers_cancel (timers, timer_id);
+    assert (rc == 0);
+    msleep (timeout * 2);
     rc = zmq_timers_execute (timers);
     assert (rc == 0);
     assert (!timer_invoked);
@@ -126,5 +218,8 @@ int main (void)
     rc = zmq_timers_destroy (&timers);
     assert (rc == 0);
 
+    test_null_timer_pointers ();
+    test_corner_cases ();
+
     return 0;
 }