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
commented
Jan 8, 2021
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
|
@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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
@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 :)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 ! :)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
AFAICS, @shihai1991, there are ref three scenarios for extension modules:
- C API: Make sure the module keeps strong refs to the capsule object for the lifetime of the capsule.
- Module state: Make sure the module keeps strong refs to the state objects for the lifetime of the state.
- 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?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
AFAICS, @shihai1991, there are ref three scenarios for extension modules:
- C API: Make sure the module keeps strong refs to the capsule object for the lifetime of the capsule.
- Module state: Make sure the module keeps strong refs to the state objects for the lifetime of the state.
- 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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This PR is stale because it has been open for 30 days with no activity.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
vstinner
Awaiting requested review from vstinner
corona10
Awaiting requested review from corona10
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
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.
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.
|