GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-80931: Skip some socket tests while hunting for refleaks on macOS by ronaldoussoren · Pull Request #114057 · python/cpython

Conversation

@ronaldoussoren

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: test_socket leaks file descriptors on macOS #80931

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ronaldoussoren
pythongh-80931: Skip some socket tests while hunting for refleaks on …
87330e7 
…macOS
@ronaldoussoren ronaldoussoren added the DO-NOT-MERGE label Jan 14, 2024
@bedevere-app bedevere-app Bot added tests Tests in the Lib/test dir awaiting core review labels Jan 14, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 14, 2024
test_socket leaks file descriptors on macOS #80931
Open
ronaldoussoren
ronaldoussoren commented Jan 14, 2024
View reviewed changes
Comment thread Lib/test/test_socket.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronaldoussoren
ronaldoussoren marked this pull request as draft January 14, 2024 13:55
@bedevere-app bedevere-app Bot removed the awaiting core review label Jan 14, 2024
@corona10
corona10 requested a review from vstinner January 14, 2024 14:13
corona10
corona10 reviewed Jan 14, 2024
View reviewed changes
Comment thread Lib/test/test_socket.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
ronaldoussoren added 2 commits January 14, 2024 15:34
@ronaldoussoren
Cleaner way of annotating test functions using a decorator
1fc2443
@ronaldoussoren
Integrate the refleak run check into regrtest and test.support
8907efc
@ronaldoussoren

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronaldoussoren
ronaldoussoren marked this pull request as ready for review January 14, 2024 20:46
@bedevere-app bedevere-app Bot added the awaiting core review label Jan 14, 2024
corona10
corona10 approved these changes Jan 16, 2024
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

For me, LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jan 16, 2024
@ronaldoussoren ronaldoussoren removed the DO-NOT-MERGE label Jan 18, 2024
Eclips4
Eclips4 approved these changes Jan 20, 2024
View reviewed changes
Eclips4
Eclips4 reviewed Jan 20, 2024
View reviewed changes
Comment thread Lib/test/test_socket.py
except ImportError:
_socket = None

def skipForRefleakHuntinIf(condition, issueref):

@Eclips4 Eclips4 Jan 20, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ncoghlan

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronaldoussoren ronaldoussoren added the skip news label Jan 21, 2024
@ronaldoussoren

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.
ncoghlan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronaldoussoren
Merge branch 'main' into pythongh-80931
86ff25a
@ronaldoussoren
ronaldoussoren merged commit 5adff3f into python:main Jan 21, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Jan 21, 2024
@ronaldoussoren
ronaldoussoren deleted the gh-80931 branch January 21, 2024 10:25
@ronaldoussoren 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

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 21, 2024
@ronaldoussoren @miss-islington
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 <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 21, 2024
@ronaldoussoren @miss-islington
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 <[email protected]>
@bedevere-app

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.11 only security fixes label Jan 21, 2024
@bedevere-app

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jan 21, 2024
@itamaro itamaro mentioned this pull request Jan 21, 2024
Switch the macOS refleaks builder back to NoGIL python/buildmaster-config#457
Merged
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
@ronaldoussoren @kulikjak
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
@ronaldoussoren @aisk
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

ambv commented Feb 21, 2024

Copy link
Copy Markdown
Contributor

I don't think this should be backported?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronaldoussoren

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@ronaldoussoren @Glyphack
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 hugovk mentioned this pull request Sep 29, 2026
[3.12] gh-148508: Add another common pattern for iOS SSL failures to test_ssl (GH-150442) #158246
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@ronaldoussoren @ncoghlan @ambv @corona10 @Eclips4
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.