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

Unwrap task function if wrapped #67

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

Conversation

marginalhours
Copy link

@marginalhours marginalhours commented Jun 14, 2021

What is the current behavior?

I made a small example of the Task API and noticed this weird behaviour: If the process where I was running queue.run() from didn't explicitly import the @queue.task-decorated function (even if unused), then instead of executing tasks instead it would endlessly queue new tasks.

This is because (I think) the wrapper gets registered instead of the function, so invoking it actually queues up a new task instead of invoking the task that's there.

What is the new behavior?

When storing a function in the registry, the hasattr check means that the original function (at least, one level of the original function - not sure about stacked decorators, could use a while: loop?) will be used instead of the wrapper. This fixes the problem locally.

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

^ will work on the above now but since it's a small patch I wanted to get this open to get people's thoughts

Thanks,

Tom

@stas
Copy link
Collaborator

stas commented Jul 21, 2021

Hmm, @MisterKeefe it would be amazing if you could provide an example how to reproduce this.

It never happened to me. Thank you!

@marginalhours
Copy link
Author

@stas Sure, here you go: https://github.com/MisterKeefe/pq-issue-replication

Let me know if any issues running it! Quite possible it's a weirdness with the way I've set things up :D

@malthe
Copy link
Owner

malthe commented Jul 21, 2021

@stas would be nice to have a test on this behavior I think.

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.

3 participants