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

Enforce functools.wraps on all monkeypatches #421

Closed
mumumumu opened this issue Jul 10, 2019 · 6 comments · Fixed by #432
Closed

Enforce functools.wraps on all monkeypatches #421

mumumumu opened this issue Jul 10, 2019 · 6 comments · Fixed by #432

Comments

@mumumumu
Copy link

mumumumu commented Jul 10, 2019

I'm using celery-once to make sure some tasks are only run one at a time. The lock is acquired in apply_async and released in after_return.

When I use the CeleryIntegration I'm not seeing any of the locks being released after the task has finished running. I'm not very familiar with the inner workings of celery, but any ideas what's going on?

@untitaker
Copy link
Member

Do you see this bug in Raven-Python? Raven uses official Celery hooks while this SDK aggressively (for lack of a better word) monkeypatches Celery to capture exceptions happening in hooks, and to work around some bugs in Celery 3.

@mumumumu
Copy link
Author

I think I've narrowed it down to an issue with celery-once. The library uses the task name and call arguments from the run method (using `inspect.getcallargs) to generate the lock key. It generates the key both when acquiring and releasing the lock. It seems the arguments before and after the task gets patched are slightly different (the wrapper takes in optional args).

I don't think this is strictly an issue with the SDK, so I'll close this for now, thanks for looking at it though.

@untitaker
Copy link
Member

@mumumumu if it's a missing functools.wraps somewhere we can fix it in the SDK as well, no big deal

@mumumumu
Copy link
Author

Thanks @untitaker, that'd be much appreciated. Adding a @wraps(f) to the _wrap_task_call _inner function does fix the issue for me.

@mumumumu mumumumu reopened this Jul 10, 2019
@untitaker untitaker changed the title CeleryIntegration does not work with tasks with an after_return handler Enforce functools.wraps on all monkeypatches Jul 10, 2019
@untitaker
Copy link
Member

We should do this everywhere then. Monkeypatches are quite inconsistent across the codebase.

@untitaker
Copy link
Member

Since this is blocking you I'll just get a minimal fix out of the door.

Proper wrapping will likely take more effort (possibly using wrapt instead of functools).

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 a pull request may close this issue.

3 participants