diff --git a/src/timers.cpp b/src/timers.cpp index c5471cb6..1474ac2a 100644 --- a/src/timers.cpp +++ b/src/timers.cpp @@ -31,11 +31,10 @@ along with this program. If not, see . #include "timers.hpp" #include "err.hpp" -zmq::timers_t::timers_t () : -tag (0xCAFEDADA), -next_timer_id (0) -{ +#include +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; diff --git a/src/timers.hpp b/src/timers.hpp index b4b40fc3..b7c5ce65 100644 --- a/src/timers.hpp +++ b/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; }; } diff --git a/tests/test_timers.cpp b/tests/test_timers.cpp index 609b7e79..4f7fc3c0 100644 --- a/tests/test_timers.cpp +++ b/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; }