FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-80931: Skip some socket tests while hunting for refleaks on macOS by ronaldoussoren · Pull Request #114057 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-80931: Skip some socket tests while hunting for refleaks on macOS - #114057

Merged
ronaldoussoren merged 4 commits into
python:mainfrom
ronaldoussoren:gh-80931
Jan 21, 2024
ConversationCommits (4)ChecksFiles changed
Merged

gh-80931: Skip some socket tests while hunting for refleaks on macOS#114057
ronaldoussoren merged 4 commits into
python:mainfrom
ronaldoussoren:gh-80931

Conversation

ronaldoussoren commented Jan 14, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

bedevere-app Bot added tests Tests in the Lib/test dir awaiting core review labels Jan 14, 2024
bedevere-app Bot mentioned this pull request Jan 14, 2024
Open
ronaldoussoren commented Jan 14, 2024
View reviewed changes
Comment thread Lib/test/test_socket.py Outdated
ronaldoussoren marked this pull request as draft January 14, 2024 13:55
corona10 requested a review from vstinner January 14, 2024 14:13
corona10 reviewed Jan 14, 2024
View reviewed changes
Comment thread Lib/test/test_socket.py

ronaldoussoren commented Jan 14, 2024

Copy link
Copy Markdown
Contributor Author

@vstinner, what's your opinion on this approach?

  • Have test.support.refleak_helper.hunting_for_refleaks() that runs true when (surprise!) hunting for refleaks
  • Have test.libregrtest.refleaks maintain the flag used by this function

It might be better to move the skipForRefleakHuntinIf in test_socket to refleak_helper as well (while removing the client_skip functionality), although that requires some additional changes to test_socket.

ronaldoussoren marked this pull request as ready for review January 14, 2024 20:46
corona10 approved these changes Jan 16, 2024
View reviewed changes

corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For me, LGTM

Eclips4 reviewed Jan 20, 2024
View reviewed changes
Comment thread Lib/test/test_socket.py
except ImportError:
_socket = None

def skipForRefleakHuntinIf(condition, issueref):

Eclips4 Jan 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe it's make sense to store it in test.support.refleak_helper (as you said above)? It's can be useful in future in other situations like this

ncoghlan commented Jan 21, 2024

Copy link
Copy Markdown
Contributor

I thought I had spotted a potential real FD leak in the "sendAncillaryIfPossible" helper method:

cpython/Lib/test/test_socket.py

Line 3762 in 8907efc

def sendAncillaryIfPossible(self, msg, ancdata):

If the message sending fails (or the ancillary data otherwise isn't passed correctly), the included FDs are never passed to SCMRightsTest.registerRecvmsgResult and hence never scheduled for cleanup.

However, any FDs allocated by the newFDs helper are already registered for cleanup regardless of whether they're received or not, so I think that's a red herring.

Is it possible we're provoking a false alarm in the fd leak hunting somehow?

ronaldoussoren commented Jan 21, 2024

Copy link
Copy Markdown
Contributor Author

Is it possible we're provoking a false alarm in the fd leak hunting somehow?

Not impossible, but unlikely given that this only is a problem on macOS in a particular scenario. I'll work on a reproducer in C for the issue, that would rule out problems in our code and is needed to file a bug with Apple about this.

ronaldoussoren merged commit 5adff3f into python:main Jan 21, 2024
ronaldoussoren deleted the gh-80931 branch January 21, 2024 10:25
ronaldoussoren added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 21, 2024

miss-islington-app Bot commented Jan 21, 2024

Copy link
Copy Markdown

Thanks @ronaldoussoren for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington-app Bot commented Jan 21, 2024

Copy link
Copy Markdown

Thanks @ronaldoussoren for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 21, 2024
pythongh-80931: Skip some socket tests while hunting for refleaks on …
9787300 
…macOS (pythonGH-114057)

Some socket tests related to sending file descriptors cause a file descriptor leak on macOS, all of them tests that send one or more descriptors than cannot be received on the read end.  This appears to be a platform bug.

This PR skips those tests when doing a refleak test run to avoid hiding other problems.
(cherry picked from commit 5adff3f)

Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 21, 2024
pythongh-80931: Skip some socket tests while hunting for refleaks on …
ae908fa 
…macOS (pythonGH-114057)

Some socket tests related to sending file descriptors cause a file descriptor leak on macOS, all of them tests that send one or more descriptors than cannot be received on the read end.  This appears to be a platform bug.

This PR skips those tests when doing a refleak test run to avoid hiding other problems.
(cherry picked from commit 5adff3f)

Co-authored-by: Ronald Oussoren <ronaldoussoren@mac.com>

bedevere-app Bot commented Jan 21, 2024

Copy link
Copy Markdown

GH-114380 is a backport of this pull request to the 3.11 branch.

bedevere-app Bot removed the needs backport to 3.11 only security fixes label Jan 21, 2024

bedevere-app Bot commented Jan 21, 2024

Copy link
Copy Markdown

GH-114381 is a backport of this pull request to the 3.12 branch.

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jan 21, 2024
itamaro mentioned this pull request Jan 21, 2024
Merged
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
pythongh-80931: Skip some socket tests while hunting for refleaks on …
9e2c953 
…macOS (python#114057)

Some socket tests related to sending file descriptors cause a file descriptor leak on macOS, all of them tests that send one or more descriptors than cannot be received on the read end.  This appears to be a platform bug.

This PR skips those tests when doing a refleak test run to avoid hiding other problems.
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
pythongh-80931: Skip some socket tests while hunting for refleaks on …
1ffa06e 
…macOS (python#114057)

Some socket tests related to sending file descriptors cause a file descriptor leak on macOS, all of them tests that send one or more descriptors than cannot be received on the read end.  This appears to be a platform bug.

This PR skips those tests when doing a refleak test run to avoid hiding other problems.

ambv commented Feb 21, 2024

Copy link
Copy Markdown
Contributor

I don't think this should be backported?

ronaldoussoren commented Feb 22, 2024

Copy link
Copy Markdown
Contributor Author

I don't think this should be backported?

TBH I haven't checked if the issue is actually present in 3.11 and 3.12. I'd expect that the issue is present there as well unless the problematic tests are only present on main. I'll check during the weekend.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
pythongh-80931: Skip some socket tests while hunting for refleaks on …
2268acc 
…macOS (python#114057)

Some socket tests related to sending file descriptors cause a file descriptor leak on macOS, all of them tests that send one or more descriptors than cannot be received on the read end.  This appears to be a platform bug.

This PR skips those tests when doing a refleak test run to avoid hiding other problems.
hugovk mentioned this pull request Sep 29, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Eclips4 Eclips4 approved these changes
corona10 corona10 approved these changes
vstinner Awaiting requested review from vstinner

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL