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-103092: Isolate socket by erlend-aasland · Pull Request #103094 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Mar 28, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Isolate Stdlib Extension Modules #103092

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
erlend-aasland added 16 commits March 28, 2023 23:17
@erlend-aasland
Establish global state
4a7c6d9
@erlend-aasland
Add sock_type to state
220a0aa
@erlend-aasland
Add exceptions to state
94ca00c
@erlend-aasland
Add accept4_works to state
fc9cdce
@erlend-aasland
Add sock_cloexec_works to state
29b4d1c
@erlend-aasland
Add defaulttimeout to state
93a2b62
@erlend-aasland
Refactor: replace state query with param in set_herror()
4164f96
@erlend-aasland
Refactor: replace state query with param in set_gaierror()
dc740d1
@erlend-aasland
Refactor: replace state query with param in new_sockobject()
abf00c3
@erlend-aasland
Refactor: replace state query with param in init_sockobject()
9c1c9c6
@erlend-aasland
Refactor: replace state query with param in gethost_common()
82c00c4
@erlend-aasland
Refactor: replace state query with param in sock_get_api()
72e69e3
@erlend-aasland
Refactor: replace state query with param in setipaddr()
274c412
@erlend-aasland
Add state pointer to PySocketSockObject
d839af0
@erlend-aasland
Use sock pointer from sock type context where applicable
13d14da
@erlend-aasland
Multi-phase init
b4edf64
@bedevere-bot bedevere-bot mentioned this pull request Mar 28, 2023
Isolate Stdlib Extension Modules #103092
Closed
24 of 25 tasks
@bedevere-bot bedevere-bot added the awaiting core review label Mar 28, 2023
kumaraditya303
kumaraditya303 reviewed Apr 4, 2023
View reviewed changes
Comment thread Modules/socketmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 reviewed Apr 4, 2023
View reviewed changes
Comment thread Modules/socketmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 approved these changes Apr 4, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Apr 4, 2023
@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 4, 2023
@bedevere-bot

bedevere-bot commented Apr 4, 2023

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 4, 2023
erlend-aasland added 4 commits April 4, 2023 15:15
@erlend-aasland
Remove unneeded forward decl
c51a696
@erlend-aasland
Add test
d8e448f
@erlend-aasland
fixup dealloc
4332571
@erlend-aasland
Add NEWS
6a00c54
erlend-aasland added 2 commits April 4, 2023 15:15
@erlend-aasland
Style: remove unneeded parens
e5027c2
@erlend-aasland
Pull in main
98d1566
@erlend-aasland

erlend-aasland commented Apr 4, 2023

Copy link
Copy Markdown
Contributor Author

There's some leaks that needs to be adressed:

$ ./python.exe measure.py
before=46451, after=47809
before=47811, after=48549
before=48549, after=49287
before=49287, after=50025
before=50025, after=50763
measure.py 
import gc
import sys

for _ in range(5):
    gc.collect()
    before = sys.gettotalrefcount()

    import _socket
    del sys.modules["_socket"]
    del _socket

    gc.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 -> 1024
0:00:00 load avg: 1.96 Run tests sequentially
0:00:00 load avg: 1.96 [1/1] test_socket
beginning 9 repetitions
123456789
.........
test_socket leaked [10, 10, 10, 10] file descriptors, sum=40
test_socket failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_socket

Total duration: 1.8 sec
Tests result: FAILURE
$ ./python.exe -m test -R : test_socket -m RecvmsgSCMRightsStreamTest                                                                                  Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 2.52 Run tests sequentially
0:00:00 load avg: 2.52 [1/1] test_socket
beginning 9 repetitions
123456789
.........
test_socket leaked [10, 10, 10, 10] file descriptors, sum=40
test_socket failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_socket

Total duration: 1.8 sec
Tests result: FAILURE
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Apr 4, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Apr 4, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Apr 5, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland added 2 commits April 8, 2023 21:38
@erlend-aasland
Sync with main
e5827e7
@erlend-aasland
Simplify ADD_EXC macro further
2019a93
@erlend-aasland
erlend-aasland marked this pull request as ready for review April 8, 2023 19:39
@erlend-aasland
erlend-aasland requested a review from kumaraditya303 April 8, 2023 19:40
@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 8, 2023
@bedevere-bot

bedevere-bot commented Apr 8, 2023

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 8, 2023
kumaraditya303
kumaraditya303 approved these changes Apr 9, 2023
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@kumaraditya303
kumaraditya303 merged commit f329a8b into python:main Apr 9, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 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
@erlend-aasland @warsaw
pythongh-103092: Isolate socket module (python#103094)
de95bcb
@CharlieZhao95 CharlieZhao95 mentioned this pull request Apr 12, 2023
gh-103092: Isolate _decimal #103381
Closed
4 of 5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

3 participants

@erlend-aasland @bedevere-bot @kumaraditya303
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.