Problem: missing tests for zmq_timers_* corner cases, missing handling of such corner cases, code duplication, missing assertions in test code

Solution: add tests, add checks to timers_t, add match_by_id functor, add assertions

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 ()
@@ -51,6 +50,11 @@ bool zmq::timers_t::check_tag ()
 
 int zmq::timers_t::add (size_t interval_, timers_timer_fn handler_, void *arg_)
 {
+    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));
@@ -58,11 +62,31 @@ int zmq::timers_t::add (size_t interval_, timers_timer_fn handler_, void* arg_)
     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,8 +98,10 @@ 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_) {
+    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_;
@@ -84,15 +110,17 @@ int zmq::timers_t::set_interval (int timer_id_, size_t interval_)
 
         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_) {
+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);
@@ -100,7 +128,6 @@ int zmq::timers_t::reset (int timer_id_) {
 
         return 0;
     }
-    }
 
     errno = EINVAL;
     return -1;
@@ -113,7 +140,8 @@ long zmq::timers_t::timeout ()
     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);
@@ -141,7 +169,8 @@ int zmq::timers_t::execute ()
     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 ()) {

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

@@ -101,6 +101,45 @@ void test_null_timer_pointers ()
     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);
+
+    rc = zmq_timers_destroy (&timers);
+    assert (rc == 0);
+}
+
 int main (void)
 {
     setup_test_environment ();
@@ -119,7 +158,9 @@ int main (void)
     assert (!timer_invoked);
 
     //  Wait half the time and check again
-    msleep (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);
@@ -131,7 +172,8 @@ int main (void)
     timer_invoked = false;
 
     //  Wait half the time and check again
-    long timeout = zmq_timers_timeout (timers);
+    timeout = zmq_timers_timeout (timers);
+    assert (rc != -1);
     msleep (timeout / 2);
     rc = zmq_timers_execute (timers);
     assert (rc == 0);
@@ -139,6 +181,7 @@ int main (void)
 
     // Reset timer and wait half of the time left
     rc = zmq_timers_reset (timers, timer_id);
+    assert (rc == 0);
     msleep (timeout / 2);
     rc = zmq_timers_execute (timers);
     assert (rc == 0);
@@ -151,7 +194,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);
@@ -159,7 +203,9 @@ int main (void)
 
     // cancel timer
     timeout = zmq_timers_timeout (timers);
-    zmq_timers_cancel (timers, timer_id);
+    assert (rc != -1);
+    rc = zmq_timers_cancel (timers, timer_id);
+    assert (rc == 0);
     msleep (timeout * 2);
     rc = zmq_timers_execute (timers);
     assert (rc == 0);
@@ -169,6 +215,7 @@ int main (void)
     assert (rc == 0);
 
     test_null_timer_pointers ();
+    test_corner_cases ();
 
     return 0;
 }