Skip to content

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
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

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

Copy link
Copy Markdown
Contributor

@bedevere-appbedevere-appBot added tests Tests in the Lib/test dir awaiting core review labels Jan 14, 2024
@bedevere-appbedevere-appBot mentioned this pull request Jan 14, 2024
Open
ronaldoussoren
ronaldoussoren commented Jan 14, 2024
View reviewed changes
Comment threadLib/test/test_socket.py Outdated
@ronaldoussoren
ronaldoussoren marked this pull request as draft January 14, 2024 13:55
corona10
corona10 reviewed Jan 14, 2024
View reviewed changes
Comment threadLib/test/test_socket.py
@ronaldoussoren

ronaldoussoren commented Jan 14, 2024

Copy link
Copy Markdown
ContributorAuthor

@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
ronaldoussoren marked this pull request as ready for review January 14, 2024 20:46
corona10
corona10 approved these changes Jan 16, 2024
View reviewed changes

@corona10corona10 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.

For me, LGTM

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

def skipForRefleakHuntinIf(condition, issueref):

@Eclips4Eclips4Jan 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.

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

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

defsendAncillaryIfPossible(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

ronaldoussoren commented Jan 21, 2024

Copy link
Copy Markdown
ContributorAuthor

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
ronaldoussoren merged commit 5adff3f into python:mainJan 21, 2024
@ronaldoussoren
ronaldoussoren deleted the gh-80931 branch January 21, 2024 10:25
@ronaldoussorenronaldoussoren 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-appBot 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

miss-islington-appBot 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
@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 <ronaldoussoren@mac.com>
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 <ronaldoussoren@mac.com>
@bedevere-app

bedevere-appBot commented Jan 21, 2024

Copy link
Copy Markdown

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

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

bedevere-appBot commented Jan 21, 2024

Copy link
Copy Markdown

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

@bedevere-appbedevere-appBot removed the needs backport to 3.12 only security fixes label Jan 21, 2024
@itamaroitamaro mentioned this pull request Jan 21, 2024
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?

@ronaldoussoren

ronaldoussoren commented Feb 22, 2024

Copy link
Copy Markdown
ContributorAuthor

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
@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.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Eclips4Eclips4Eclips4 approved these changes
@corona10corona10corona10 approved these changes
@vstinnervstinnerAwaiting requested review from vstinner

Assignees

No one assigned

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ronaldoussoren@ncoghlan@ambv@corona10@Eclips4