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

gh-131233: remove return-in-finally in multiprocessing/connection.py #131416

Merged
merged 3 commits into from
Mar 21, 2025

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Mar 18, 2025

@markshannon
Copy link
Member

I'm struggling to see any difference in behavior here, so:

  • I'm not understanding this, or
  • the original code wasn't buggy (I think that is the case), or
  • this doesn't fix the bug.

Or is this just to fix the warning and not change the semantics?

@encukou
Copy link
Member

encukou commented Mar 21, 2025

The warning causes a flaky test, see https://buildbot.python.org/#/builders/29/builds/7932

@nascheme
Copy link
Member

I agree the "is" and "is not" would be slightly better then ==. Also, I think it helps to "push down" the code that's inside the outer try/except into a helper method. The triple nested try/except is quite hard to understand.

@iritkatriel
Copy link
Member Author

I agree the "is" and "is not" would be slightly better then ==. Also, I think it helps to "push down" the code that's inside the outer try/except into a helper method. The triple nested try/except is quite hard to understand.

This can be done as a further improvement. In this PR I'd rather make the change more straightforward to see from the diff.

@iritkatriel iritkatriel enabled auto-merge (squash) March 21, 2025 17:42
@markshannon
Copy link
Member

The warning causes a flaky test, see https://buildbot.python.org/#/builders/29/builds/7932

The issue says there is a bug, not just a warning. What's the bug?

@iritkatriel
Copy link
Member Author

The warning causes a flaky test, see https://buildbot.python.org/#/builders/29/builds/7932

The issue says there is a bug, not just a warning. What's the bug?

See discussion on the issue - assumption now is that it's not a bug. I'll fix the title.

@iritkatriel iritkatriel merged commit 3e2ccea into python:main Mar 21, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants