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

Woraround for registry resolving when restarting celery prefork workers #589

Closed
wants to merge 3 commits into from

Conversation

perronld
Copy link
Contributor

Workaround pyramid.threadlocal:get_current_registry() when restarting Celery workers processes (e.g. --max-tasks-per-child)

@perronld perronld requested a review from fmigneault November 15, 2023 21:58
@github-actions github-actions bot added the ci/doc Issue related to documentation of the package label Nov 15, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for submitting a PR.
Make sure that you have added tests to validates new features or bugfixes. Also, ensure that existing test suites are still working with the change using the commit test statuses.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e5f79a5) 85.18% compared to head (d85cf99) 85.19%.

❗ Current head d85cf99 differs from pull request most recent head ab1b834. Consider uploading reports for the commit ab1b834 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
+ Coverage   85.18%   85.19%   +0.01%     
==========================================
  Files          79       79              
  Lines       18023    18026       +3     
  Branches     2728     2729       +1     
==========================================
+ Hits        15353    15358       +5     
+ Misses       1940     1939       -1     
+ Partials      730      729       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@fmigneault fmigneault left a comment

Choose a reason for hiding this comment

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

How does it make a workaround?

The doc seems to indicate that get_current_request is just as bad as get_current_registry.
https://docs.pylonsproject.org/projects/pyramid/en/latest/api/threadlocal.html

Also, what happens if the code is not running with celery (ie: API or CLI), and get_registry() is called. Wouldn't that possibly create an endless recursion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/doc Issue related to documentation of the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants