-
Notifications
You must be signed in to change notification settings - Fork 262
Resolve Celery TaskObject Race Condition #4268
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
Resolve Celery TaskObject Race Condition #4268
Conversation
b77dd03 to
224a05c
Compare
|
|
||
| super_as_dict = TaskResult.as_dict | ||
|
|
||
| def as_dict(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth checking if our own code calls this, as I overrode on TaskResult to add in the additional fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edited my previous comment: missing the word 'code'. @ozer550 did you check to see if this was used at all? This was used internally to django-celery-results, and overriding it allowed us to add our custom fields to the dict.
bjester
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some important notes on the CeleryAsyncResult properties (if they're actually being used still)
Summary
Description of the change(s) you made
Manual verification steps performed
Does this introduce any tech-debt items?
The only thing that comes to mind is find_incomplete_ids function being executed before the celery TaskObject is saved to the backend asynchronously. This edge case is very difficult to happen relatively as there should surely be some latency due to request, which would ensure the saving of the celery TaskObject in the backend safely. Just to give out some figures the test test_revoke_task when not having a delay between enqueuing and revoking the same tasks creates the above-mentioned issue but if we add a delay of just 0.5 seconds this edge case is not caused.
References
closes #4222
Comments
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)WIP