Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parameter for on_error to BusABC.send_periodic #1283

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andebjor
Copy link

If there is an error while sending messages in a (background) periodic
task, then the on_error callback is called, if set. If the callback is
configured and returns True then the task continues, otherwise it is
aborted.

While it is possible to update a task with a callback after the task has
been created, this procedure is prone to a race condition where the
callback might not be configured in time for the first send event. Thus,
if the first send fails and the callback has not yet been configured,
the task will abort.

This commit solves the race condition issue by adding an argument to
BusABC.send_periodic to specify the callback. By including the
callback in the constructor it will be deterministically active for all
sends in the task. This fixes issue #1282.

This commit also adds myself to the CONTRIBUTORS list.

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #1283 (a0e1f92) into develop (af55b0a) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1283   +/-   ##
========================================
  Coverage    66.02%   66.02%           
========================================
  Files           86       86           
  Lines         8915     8915           
========================================
  Hits          5886     5886           
  Misses        3029     3029           

If there is an error while sending messages in a (background) periodic
task, then the `on_error` callback is called, if set. If the callback is
configured and returns `True` then the task continues, otherwise it is
aborted.

While it is possible to update a task with a callback after the task has
been created, this procedure is prone to a race condition where the
callback might not be configured in time for the first send event. Thus,
if the first send fails and the callback has not yet been configured,
the task will abort.

This commit solves the race condition issue by adding an argument to
`BusABC.send_periodic` to specify the callback. By including the
callback in the constructor it will be deterministically active for all
sends in the task. This fixes issue hardbyte#1282.

This commit also adds myself to the CONTRIBUTORS list.
@andebjor andebjor force-pushed the periodic_callback_arg branch from 3b8bb3c to a0e1f92 Compare March 16, 2022 08:18
@@ -232,7 +240,7 @@ def send_periodic(
# Create a backend specific task; will be patched to a _SelfRemovingCyclicTask later
task = cast(
_SelfRemovingCyclicTask,
self._send_periodic_internal(msgs, period, duration),
self._send_periodic_internal(msgs, period, duration, on_error),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail for subclasses that reimplement _send_periodic_internal()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants