Skip to content

gh-103092: Isolate socket - #103094

Merged
kumaraditya303 merged 24 commits into
python:mainfrom
erlend-aasland:isolate-socket
Apr 9, 2023
Merged

gh-103092: Isolate socket#103094
kumaraditya303 merged 24 commits into
python:mainfrom
erlend-aasland:isolate-socket

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Mar 28, 2023

Copy link
Copy Markdown
Contributor

Comment threadModules/socketmodule.c Outdated
Comment threadModules/socketmodule.c Outdated
@erlend-aaslanderlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 4, 2023
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit b4edf64 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 4, 2023
@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

There's some leaks that needs to be adressed:

$ ./python.exe measure.pybefore=46451, after=47809before=47811, after=48549before=48549, after=49287before=49287, after=50025before=50025, after=50763
measure.py
importgcimportsysfor_inrange(5):
gc.collect()
before=sys.gettotalrefcount()
import_socketdelsys.modules["_socket"]
del_socketgc.collect()
after=sys.gettotalrefcount()
print(f"{before=}, {after=}")
# assert after == before

... and these two:

$ ./python.exe -m test -R : test_socket -m RecvmsgIntoSCMRightsStreamTest Raised RLIMIT_NOFILE: 256 -> 10240:00:00 load avg: 1.96 Run tests sequentially0:00:00 load avg: 1.96 [1/1] test_socketbeginning 9 repetitions123456789.........test_socket leaked [10, 10, 10, 10] file descriptors, sum=40test_socket failed (reference leak)== Tests result: FAILURE ==1 test failed: test_socketTotal duration: 1.8 secTests result: FAILURE
$ ./python.exe -m test -R : test_socket -m RecvmsgSCMRightsStreamTest Raised RLIMIT_NOFILE: 256 -> 10240:00:00 load avg: 2.52 Run tests sequentially0:00:00 load avg: 2.52 [1/1] test_socketbeginning 9 repetitions123456789.........test_socket leaked [10, 10, 10, 10] file descriptors, sum=40test_socket failed (reference leak)== Tests result: FAILURE ==1 test failed: test_socketTotal duration: 1.8 secTests result: FAILURE

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

Regarding #103094 (comment): the file descriptors leak on main as well, so it is not a regression introduced by this PR. Let's not bother with those. There's still references leaked by repeated init of the module, though.

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

Seems like the leaks are due to the capsulated C API not being freed.

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

I'd like to hold this PR until #103261 has landed.

@erlend-aasland
erlend-aasland marked this pull request as ready for review April 8, 2023 19:39
@erlend-aaslanderlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 8, 2023
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 2019a93 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 8, 2023

@kumaraditya303kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@kumaraditya303
kumaraditya303 merged commit f329a8b into python:mainApr 9, 2023
@kumaraditya303
kumaraditya303 deleted the isolate-socket branch April 9, 2023 01:03
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@CharlieZhao95CharlieZhao95 mentioned this pull request Apr 12, 2023
5 tasks
Sign up for freeto 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

@erlend-aasland@bedevere-bot@kumaraditya303