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 — bpo-1635741: Convert socket.SocketType to heap type, and establish global state by erlend-aasland · Pull Request #24175 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jan 8, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Prepare the socket module for multi-phase init by converting socket.SocketType
to heap type, establish a global module state, and clean up the first part of the
init function.

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

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

All reactions
Erlend E. Aasland added 3 commits January 8, 2021 22:52
Erlend E. Aasland
Convert socket to heap type
5c1533b
Erlend E. Aasland
Establish global state
fc923e7
Erlend E. Aasland
Clean up first part of init function
b41d9aa
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 8, 2021
@bedevere-bot bedevere-bot added the awaiting review label Jan 8, 2021
@erlend-aasland

erlend-aasland commented Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

@vstinner Do I need to create SocketType as a GC type with traverse function in this case? I can't find ref leaks with python.exe -m test -R 3:5 test_socket.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

@shihai1991 / @corona10: In commit b41d9aa (clean up first part of init function), I assume the following:
When an object is successfully added to a module using PyModule_AddObjectRef, it is added to the module dict, which means that the module dict has a strong reference to the object, right? This also means that when the module is destroyed, the module dict, together with it's values, are also destroyed, right? Unless I'm missing something, I think this commit is ref leak safe :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@shihai1991
shihai1991 requested review from corona10 and vstinner January 10, 2021 14:09
@shihai1991

shihai1991 commented Jan 10, 2021

Copy link
Copy Markdown
Member

When an object is successfully added to a module using PyModule_AddObjectRef, it is added to the module dict, which means that the module dict has a strong reference to the object, right?

You are right :).

This also means that when the module is destroyed, the module dict, together with it's values, are also destroyed, right? Unless I'm missing something, I think this commit is ref leak safe :)

You are right, the object's dealloc() will be triaggered when the object's ref is zero. But the extension module's state can be take cared by the GC(it means that you don't need to free them manually). You can take a look m_traverse, m_free and m_clear in https://docs.python.org/3.10/c-api/module.html?highlight=m_traverse#c.PyModuleDef.m_traverse.

All reactions

Sorry, something went wrong.

Uh oh!

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

@shihai1991 shihai1991 added the skip news label Jan 10, 2021
@erlend-aasland

erlend-aasland commented Jan 10, 2021

Copy link
Copy Markdown
Contributor Author

When an object is successfully added to a module using PyModule_AddObjectRef, it is added to the module dict, which means that the module dict has a strong reference to the object, right?
You are right :).

So, that means I only need to employ a strong ref to protect the object while calling PyModule_AddObjectRef: In the ADD_OBJ_REF macro, wrap PyModule_AddObjectRef with Py_INCREF and Py_DECREF. Right?

Thank you ! :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jan 10, 2021

Copy link
Copy Markdown
Contributor Author

AFAICS, @shihai1991, there are ref three scenarios for extension modules:

  1. C API: Make sure the module keeps strong refs to the capsule object for the lifetime of the capsule.
  2. Module state: Make sure the module keeps strong refs to the state objects for the lifetime of the state.
  3. Module dict: Make sure the module keeps strong refs to the objects only until the module dict has acquired its own strong refs.

Did I miss anything?

All reactions

Sorry, something went wrong.

Uh oh!

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

@shihai1991

shihai1991 commented Jan 12, 2021

Copy link
Copy Markdown
Member

AFAICS, @shihai1991, there are ref three scenarios for extension modules:

  1. C API: Make sure the module keeps strong refs to the capsule object for the lifetime of the capsule.
  2. Module state: Make sure the module keeps strong refs to the state objects for the lifetime of the state.
  3. Module dict: Make sure the module keeps strong refs to the objects only until the module dict has acquired its own strong refs.

Did I miss anything?

No, I have no other info to supply :)

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

@github-actions

github-actions Bot commented Feb 12, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Feb 12, 2021
@erlend-aasland erlend-aasland closed this Mar 19, 2021
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed
@erlend-aasland
erlend-aasland deleted the bpo-1635741/socket-part1 branch August 22, 2023 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner Awaiting requested review from vstinner
@corona10 corona10 Awaiting requested review from corona10

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review skip news stale Stale PR or inactive for long period of time.

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.

4 participants

@erlend-aasland @shihai1991 @the-knights-who-say-ni @bedevere-bot
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.