Skip to content

Replace DelayedDelivery connection creation to use context manger #9793

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.

Sign up for GitHub

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

Jump to bottom
Merged
auvipy merged 3 commits into from
Jul 5, 2025
Merged

Replace DelayedDelivery connection creation to use context manger #9793

auvipy merged 3 commits into from
Jul 5, 2025

Conversation

@bityob
Copy link
Contributor

@bityob bityob commented Jul 3, 2025

Description

  • Replaced DelayedDelivery step connection creation to use context manager, so the the connection will be closed when the step is completed.

Fixing an issue I found - #9792.

Verifications

Running Celery with my fix, created the exchange and queues needed with binding, and closed the connection.

uvx --no-cache --with /home/bit/Code/celery celery -A app worker --loglevel=debug --without-gossip --without-mingle --without-heartbeat --prefetch-multiplier=1 --concurrency=1

Logs -

image

Rabbit connections - only 1 left as expected, for consuming tasks -

image

Rabbit channels - only 1 channel -

image

Exchanges were created successfully -

image

Queues were created successfully -

image

auvipy
auvipy requested changes Jul 3, 2025
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

the code changes are making the CI fail. can you please add/update tests to verify this change?

@auvipy auvipy requested review from Nusnus and Copilot July 3, 2025 15:04
Copilot AI reviewed Jul 3, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the delayed delivery setup to open the broker connection using a context manager, ensuring it’s closed automatically when the step completes.

  • Replaced manual connection_for_write assignment with a with block for automatic closing.
  • Scoped all delayed-delivery setup logic within the context manager.
  • Resolves issue #9792 by preventing orphaned connections.
Comments suppressed due to low confidence (1)

celery/worker/consumer/delayed_delivery.py:117

  • Add a unit test that mocks connection_for_write and asserts that connection.close() is called when _setup_delayed_delivery completes to verify the new context-manager behavior.
        with c.app.connection_for_write(url=broker_url) as connection:

@bityob
Fixed failing test because of usage of Mock which doesn't support con…
10e3793 
…text manager, replaced those places with MagicMock instead
@bityob
Copy link
Contributor Author

bityob commented Jul 3, 2025 •
edited
Loading

the code changes are making the CI fail. can you please add/update tests to verify this change?

  • Those tests failed because of the usage of connection context manager that is not support seamlessly in Mock.
    Fixed by changing those cases to use MagicMock instead of Mock.
  • Also modified test_start_native_delayed_delivery_topic_exchange test, to verify the connection context was called

Verified locally by running -

cd docker
docker compose run -it --rm celery bash
pyenv exec python3.12 -m pytest t/unit/worker/test_native_delayed_delivery.py -v

image

@codecov
Copy link

codecov bot commented Jul 3, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.62%. Comparing base (a83070e) to head (ecc4715).
⚠️ Report is 43 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #9793   +/-   ##
=======================================
  Coverage   78.62%   78.62%           
=======================================
  Files         153      153           
  Lines       19178    19178           
  Branches     2540     2540           
=======================================
  Hits        15078    15078           
  Misses       3809     3809           
  Partials      291      291
Flag Coverage Δ
unittests 78.60% <100.00%> (ø)

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

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

Nusnus
Nusnus reviewed Jul 3, 2025
View reviewed changes
Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

My last change here was mainly adding better errors, but this is a much welcome improvement.

Let's see how the CI is doing and move forward, looking good in general

@auvipy auvipy added this to the 5.6.0 milestone Jul 5, 2025
auvipy
auvipy approved these changes Jul 5, 2025
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

thats good now

@auvipy auvipy merged commit 23521b1 into celery:main Jul 5, 2025
234 of 235 checks passed
@Nusnus Nusnus linked an issue Jul 29, 2025 that may be closed by this pull request
DelayedDelivery Step Connection is left open without closing #9792
Closed
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nusnus Nusnus Nusnus left review comments

Copilot code review Copilot Copilot left review comments

@auvipy auvipy auvipy approved these changes

Assignees

No one assigned

Labels

PR Type: Bugfix Worker

Projects

None yet

Milestone

5.6.0

Development

Successfully merging this pull request may close these issues.

DelayedDelivery Step Connection is left open without closing

3 participants

@bityob @Nusnus @auvipy