fix: Preserve call/errbacks of replaced tasks by maybe-sybr · Pull Request #6770 · celery/celery

maybe-sybr · 2021-05-14T05:24:16Z

Description

This change fixes the handling of callbacks and errbacks on tasks which get replaced to
ensure they get called as expected if the replacement signature succeeds or fails.

Fixes #6441

maybe-sybr · 2021-05-14T05:25:30Z

This is a bugfix but I think we can land it on master once we sort out 5.1 since it's been broken for a while. :) It'll improve my life but FWICT there are no issues from other users tracking this misbehaviour.

lgtm-com · 2021-05-14T05:59:17Z

This pull request introduces 1 alert when merging d8bf93f into 2411504 - view on LGTM.com

new alerts:

1 for Non-exception in 'except' clause

maybe-sybr · 2021-05-15T02:49:09Z

t/unit/tasks/test_tasks.py::test_tasks::test_replace_callback() is failing because I'm linking the callbacks and errbacks of the replaced signature to a replacement group even though they should should only be linked to the accumulate task that we chain onto the end of the group signature. Since a callback/errback was being set for a group, when that group was eventually applied it would die due to the TypeError() we were commenting on above:

self = group([t.unit.tasks.test_tasks.return_True()]), args = (), kwargs = None, add_to_parent = True, producer = None
link = ['callbacks'], link_error = ['errbacks']
options = {'group_id': 'group', 'group_index': None, 'root_id': 'root_id', 'task_id': '739d4d46-a741-4db5-aa5b-eacdb75de6bf'}

    def apply_async(self, args=None, kwargs=None, add_to_parent=True,
                    producer=None, link=None, link_error=None, **options):
        args = args if args else ()
        if link is not None or link_error is not None:
>           raise TypeError('Cannot add linked tasks to groups: use a chord')
E           TypeError: Cannot add linked tasks to groups: use a chord

celery/canvas.py:1076: TypeError

I've changed the logic in replace() to only link call/errbacks to the replacement signature if it's not a group. I'll push a new diff once I've done some more sanity checking on the integration tests.

codecov · 2021-05-17T00:31:54Z

Codecov Report

Merging #6770 (5370fdb) into master (536849c) will increase coverage by 0.01%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##           master    #6770      +/-   ##
==========================================
+ Coverage   70.71%   70.73%   +0.01%     
==========================================
  Files         138      138              
  Lines       16604    16605       +1     
  Branches     2094     2094              
==========================================
+ Hits        11741    11745       +4     
+ Misses       4667     4663       -4     
- Partials      196      197       +1

Flag	Coverage Δ
unittests	`70.73% <53.33%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
celery/canvas.py	`94.82% <ø> (ø)`
celery/app/task.py	`94.65% <53.33%> (+0.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 536849c...5370fdb. Read the comment docs.

lgtm-com · 2021-05-17T00:41:53Z

This pull request introduces 2 alerts and fixes 1 when merging 00d5516 into 2411504 - view on LGTM.com

new alerts:

2 for Unused import

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lgtm-com · 2021-05-17T01:04:23Z

This pull request introduces 1 alert when merging ff9f31f into 2411504 - view on LGTM.com

new alerts:

1 for Unused import

maybe-sybr · 2021-05-17T01:06:39Z

I've reworked the replacement logic entirely now. It could do with a re-review. AFAICT, removing the early replacement_sig.freeze(self.request.id) hasn't disrupted any of our unit or integration tests (against redis) so I'm reasonably confident that the diff is fine to merge.

maybe-sybr · 2021-05-17T04:18:48Z

This now behaves as I expect and we have some new integration tests to catch regressions or misbehaviours. That actually exposed an issue with chord header result ordering which I've also fixed in the commit on the tip of this branch.

lgtm-com · 2021-05-17T04:54:12Z

This pull request introduces 1 alert when merging d705a73 into 2411504 - view on LGTM.com

new alerts:

1 for Non-exception in 'except' clause

celery/app/task.py

t/integration/test_canvas.py

lgtm-com · 2021-05-18T04:43:15Z

This pull request introduces 1 alert and fixes 2 when merging 1095f05 into 2411504 - view on LGTM.com

new alerts:

1 for Unused import

fixed alerts:

1 for Non-exception in 'except' clause
1 for Module is imported with 'import' and 'import from'

maybe-sybr · 2021-05-19T00:38:27Z

That last fixup broke a unit test, it (should be) fixed again now.

maybe-sybr · 2021-05-19T23:26:23Z

LGTM.

This code is much cleaner than before.

Great thanks :) I've just marked this for 5.2 so we can come back and merge it once we drop 5.1

maybe-sybr · 2021-05-19T23:28:10Z

I'll mark this as needs rebase to remind me to rebase on top of 5.1 as prep for merge

lgtm-com · 2021-05-23T23:41:41Z

This pull request introduces 1 alert when merging fcfbe43 into 025bad6 - view on LGTM.com

new alerts:

1 for Unused import

auvipy

rebase please. as it's a bug fix we can land this on 5.1.x as well

auvipy

rebase please. as it's a bug fix we can land this on 5.1.x as well

This change modifies a bunch of the tests to use unique keys for the `redis_echo` and `redis_count` tasks which are used to validate that callbacks and errbacks are made. We also introduce helper functions for validating that messages/counts are seen to reduce duplicate code.

Fixes #6441

This change adds some tests to ensure that when a task is replaced, it runs as expected. This exposed a bug where the group index of a task would be lost when replaced with a chain since chains would not pass their `group_index` option down to the final task when applied. This manifested as the results of chords being mis-ordered on the redis backend since the group index would default to `+inf`. Other backends may have had similar issues.

maybe-sybr · 2021-06-15T02:18:48Z

rebase please. as it's a bug fix we can land this on 5.1.x as well

rebased on master

lgtm-com · 2021-06-15T02:49:35Z

This pull request introduces 2 alerts when merging 5370fdb into 536849c - view on LGTM.com

new alerts:

1 for Non-exception in 'except' clause
1 for Module is imported with 'import' and 'import from'

* style: Remove unused var from canvas unit tests * test: Check task ID re-freeze on replacement * refac: Remove duped task ID preservation logic * test: Rework canvas call/errback integration tests This change modifies a bunch of the tests to use unique keys for the `redis_echo` and `redis_count` tasks which are used to validate that callbacks and errbacks are made. We also introduce helper functions for validating that messages/counts are seen to reduce duplicate code. * fix: Preserve call/errbacks of replaced tasks Fixes celery#6441 * fix: Ensure replacement tasks get the group index This change adds some tests to ensure that when a task is replaced, it runs as expected. This exposed a bug where the group index of a task would be lost when replaced with a chain since chains would not pass their `group_index` option down to the final task when applied. This manifested as the results of chords being mis-ordered on the redis backend since the group index would default to `+inf`. Other backends may have had similar issues.

maybe-sybr added Component: Canvas PR Type: Bugfix labels May 14, 2021

maybe-sybr requested a review from thedrow May 14, 2021 05:24

maybe-sybr marked this pull request as draft May 16, 2021 23:25

maybe-sybr force-pushed the fix/signature-replace-callbacks branch from d8bf93f to 00d5516 Compare May 17, 2021 00:28

maybe-sybr force-pushed the fix/signature-replace-callbacks branch from 00d5516 to ff9f31f Compare May 17, 2021 00:47

maybe-sybr marked this pull request as ready for review May 17, 2021 01:06

maybe-sybr marked this pull request as draft May 17, 2021 02:09

maybe-sybr force-pushed the fix/signature-replace-callbacks branch from ff9f31f to d705a73 Compare May 17, 2021 04:16

maybe-sybr marked this pull request as ready for review May 17, 2021 04:16

maybe-sybr commented May 17, 2021

View reviewed changes

celery/app/task.py Outdated Show resolved Hide resolved

maybe-sybr commented May 17, 2021

View reviewed changes

t/integration/test_canvas.py Outdated Show resolved Hide resolved

maybe-sybr force-pushed the fix/signature-replace-callbacks branch 2 times, most recently from 639ba23 to 1095f05 Compare May 18, 2021 04:22

maybe-sybr mentioned this pull request May 18, 2021

test: Improve use of test fixtures in integration tests #6773

Closed

maybe-sybr force-pushed the fix/signature-replace-callbacks branch from 1095f05 to 1e74fb1 Compare May 19, 2021 00:38

maybe-sybr added this to the 5.2 milestone May 19, 2021

maybe-sybr added the Status: Ready For Merge ✔ label May 19, 2021

maybe-sybr added Status: Needs Rebase ✘ and removed Status: Ready For Merge ✔ labels May 19, 2021

maybe-sybr dismissed a stale review via fcfbe43 May 23, 2021 23:14

maybe-sybr force-pushed the fix/signature-replace-callbacks branch from 1e74fb1 to fcfbe43 Compare May 23, 2021 23:14

auvipy requested changes Jun 14, 2021

View reviewed changes

maybe-sybr added 6 commits June 15, 2021 12:18

style: Remove unused var from canvas unit tests

40c2035

test: Check task ID re-freeze on replacement

b9ef105

refac: Remove duped task ID preservation logic

1b7bdd0

fix: Preserve call/errbacks of replaced tasks

7a925fa

Fixes #6441

maybe-sybr force-pushed the fix/signature-replace-callbacks branch from fcfbe43 to 5370fdb Compare June 15, 2021 02:19

maybe-sybr removed the Status: Needs Rebase ✘ label Jun 15, 2021

auvipy approved these changes Jun 15, 2021

View reviewed changes

auvipy removed this from the 5.2 milestone Jun 15, 2021

auvipy merged commit 5d72aee into master Jun 15, 2021

auvipy added this to the 5.1.x milestone Jun 15, 2021

Uh oh!

Conversation

maybe-sybr commented May 14, 2021

Description

Uh oh!

maybe-sybr commented May 14, 2021

Uh oh!

lgtm-com bot commented May 14, 2021

Uh oh!

maybe-sybr commented May 15, 2021

Uh oh!

codecov bot commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lgtm-com bot commented May 17, 2021

Uh oh!

lgtm-com bot commented May 17, 2021

Uh oh!

maybe-sybr commented May 17, 2021

Uh oh!

maybe-sybr commented May 17, 2021

Uh oh!

lgtm-com bot commented May 17, 2021

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented May 18, 2021

Uh oh!

maybe-sybr commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maybe-sybr commented May 19, 2021

Uh oh!

maybe-sybr commented May 19, 2021

Uh oh!

lgtm-com bot commented May 23, 2021

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

auvipy left a comment

Choose a reason for hiding this comment

Uh oh!

maybe-sybr commented Jun 15, 2021

Uh oh!

lgtm-com bot commented Jun 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented May 17, 2021 •

edited

Loading

maybe-sybr commented May 19, 2021 •

edited

Loading