Skip to content

Conversation

@torarvid
Copy link
Contributor

This change helps make the code compatible with Python 3.14, where 'return' in finally blocks is no longer allowed.

This change helps make the code compatible with Python 3.14, where
'return' in finally blocks is no longer allowed.
Copy link

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 refactors code to eliminate the use of return statements within finally blocks to ensure compatibility with Python 3.14, where this pattern will no longer be allowed. The changes affect exception handling and control flow in two critical modules.

Key changes:

  • Removed finally block with return statement in billiard/pool.py:_ack() method and moved the conditional return logic outside the try-except block
  • Refactored billiard/connection.py:_recv_bytes() to use deferred return/raise pattern instead of returning from finally block

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
billiard/pool.py Simplified exception handling in _ack() by removing the finally block and moving the acknowledgment send logic after the try-except block
billiard/connection.py Refactored Windows pipe connection's _recv_bytes() to store result and exception state variables, enabling return/raise after finally block completes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

auvipy and others added 2 commits November 30, 2025 13:48
@torarvid
Copy link
Contributor Author

@auvipy Hi 👋🏻 Yes, my own analysis was that by catching BaseException, we would get similar behavior as before (since the return-in-finally before would also prevent BaseExceptions from propagating). The other option that Copilot suggests is IMO more correct. I only went for the BaseException route because I’m not familiar enough with this repo to change behavior «on my own» 😅

So these changes LGTM 👍🏼

@auvipy auvipy merged commit 973c773 into celery:main Nov 30, 2025
11 checks passed
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.

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