From 750b51caaab22632d772044a3eb8e9850b8b3ef7 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 5 Dec 2018 14:33:10 +1300 Subject: [PATCH] Add better timer APIs to ThreadGroup The ThreadGroup.add_timer() API is unintuitive, inflexible, and all around pretty terrible. By allowing the caller to pass *args and **kwargs, it strongly implies that you can write a wrapper like: def add_timer(self, interval, func, *args, **kwargs): self.group.add_timer(interval, func, *args, **kwargs) and in fact at least 6 projects have done so (probably copying and pasting from each other). But this is wrong, and will result in the first positional arg intended for the callback function to be treated as the initial_delay parameter and dropped from the list of arguments to be passed to the callback when it is run. When called like this, the initial_delay argument not only must be passed (preventing the caller from relying on the default), interspersed between the callback function and its arguments, but it must be passed as a positional and not a keyword argument (preventing the caller from effectively documenting what is going on): self.group.add_timer(interval, func, None, *args, **kwargs) We are also unable to add any further options without breaking existing consumers. To improve the situation in the future, add a new add_timer_args() API that takes the args and kwargs as individual sequence/mapping parameters rather than as variadic parameters. The above call would become: self.group.add_timer_args(interval, func, args, kwargs) and any optional parameters can be passed as keyword arguments. Any new keyword arguments we might want to add can be safely added to this method. Calling the original add_timer() method with arguments (either positional or keyword) intended for the callback function is now deprecated. Those parameters could be removed in a future major release. Change-Id: Ib2791342263e2b88c045bcc92adc8160f57a0ed6 --- oslo_service/tests/test_threadgroup.py | 43 +++++++++++++++++++ oslo_service/threadgroup.py | 27 ++++++++++++ .../notes/timer-args-f578c8f9d08b217d.yaml | 15 +++++++ 3 files changed, 85 insertions(+) create mode 100644 releasenotes/notes/timer-args-f578c8f9d08b217d.yaml diff --git a/oslo_service/tests/test_threadgroup.py b/oslo_service/tests/test_threadgroup.py index 7857e06c..a4ab26c0 100644 --- a/oslo_service/tests/test_threadgroup.py +++ b/oslo_service/tests/test_threadgroup.py @@ -49,6 +49,49 @@ class ThreadGroupTestCase(test_base.BaseTestCase): self.assertEqual(('arg',), timer.args) self.assertEqual({'kwarg': 'kwarg'}, timer.kw) + def test_add_dynamic_timer_args(self): + def foo(*args, **kwargs): + pass + + self.tg.add_dynamic_timer_args(foo, ['arg'], {'kwarg': 'kwarg'}, + initial_delay=1, + periodic_interval_max=2) + + self.assertEqual(1, len(self.tg.timers)) + + timer = self.tg.timers[0] + self.assertTrue(timer._running) + self.assertEqual(('arg',), timer.args) + self.assertEqual({'kwarg': 'kwarg'}, timer.kw) + + def test_add_timer(self): + def foo(*args, **kwargs): + pass + + self.tg.add_timer(1, foo, 1, + 'arg', kwarg='kwarg') + + self.assertEqual(1, len(self.tg.timers)) + + timer = self.tg.timers[0] + self.assertTrue(timer._running) + self.assertEqual(('arg',), timer.args) + self.assertEqual({'kwarg': 'kwarg'}, timer.kw) + + def test_add_timer_args(self): + def foo(*args, **kwargs): + pass + + self.tg.add_timer_args(1, foo, ['arg'], {'kwarg': 'kwarg'}, + initial_delay=1) + + self.assertEqual(1, len(self.tg.timers)) + + timer = self.tg.timers[0] + self.assertTrue(timer._running) + self.assertEqual(('arg',), timer.args) + self.assertEqual({'kwarg': 'kwarg'}, timer.kw) + def test_stop_current_thread(self): stop_event = event.Event() diff --git a/oslo_service/threadgroup.py b/oslo_service/threadgroup.py index ee5ad0f2..bcb79d0b 100644 --- a/oslo_service/threadgroup.py +++ b/oslo_service/threadgroup.py @@ -14,6 +14,7 @@ import logging import threading +import warnings import eventlet from eventlet import greenpool @@ -77,6 +78,20 @@ class ThreadGroup(object): def add_dynamic_timer(self, callback, initial_delay=None, periodic_interval_max=None, *args, **kwargs): + if args or kwargs: + warnings.warn("Calling add_dynamic_timer() with arguments to the " + "callback function is deprecated. Use " + "add_dynamic_timer_args() instead.", + DeprecationWarning) + return self.add_dynamic_timer_args( + callback, args, kwargs, + initial_delay=initial_delay, + periodic_interval_max=periodic_interval_max) + + def add_dynamic_timer_args(self, callback, args=None, kwargs=None, + initial_delay=None, periodic_interval_max=None): + args = args or [] + kwargs = kwargs or {} timer = loopingcall.DynamicLoopingCall(callback, *args, **kwargs) timer.start(initial_delay=initial_delay, periodic_interval_max=periodic_interval_max) @@ -85,6 +100,18 @@ class ThreadGroup(object): def add_timer(self, interval, callback, initial_delay=None, *args, **kwargs): + if args or kwargs: + warnings.warn("Calling add_timer() with arguments to the callback " + "function is deprecated. Use add_timer_args() " + "instead.", + DeprecationWarning) + return self.add_timer_args(interval, callback, args, kwargs, + initial_delay=initial_delay) + + def add_timer_args(self, interval, callback, args=None, kwargs=None, + initial_delay=None): + args = args or [] + kwargs = kwargs or {} pulse = loopingcall.FixedIntervalLoopingCall(callback, *args, **kwargs) pulse.start(interval=interval, initial_delay=initial_delay) diff --git a/releasenotes/notes/timer-args-f578c8f9d08b217d.yaml b/releasenotes/notes/timer-args-f578c8f9d08b217d.yaml new file mode 100644 index 00000000..49f7c729 --- /dev/null +++ b/releasenotes/notes/timer-args-f578c8f9d08b217d.yaml @@ -0,0 +1,15 @@ +--- +features: + - | + The ThreadGroup class has new add_timer_args() and add_dynamic_timer_args() + methods to create timers passing the positional and keyword arguments to + the callback as a sequence and a mapping. This API provides more + flexibility for the addition of timer control options in the future. +deprecations: + - | + The API of the ThreadGroup add_timer() and add_dynamic_timer() methods has + been identified as error-prone when passing arguments intended for the + callback function. Passing callback arguments in this way is now + deprecated. Callers should use the new add_timer_args() or + add_dynamic_timer_args() methods (respectively) instead when it is + necessary to pass arguments to the timer callback function.