Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-1635741: Convert socket.SocketType to heap type, and establish global state #24175

Closed

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 8, 2021

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

@erlend-aasland
Copy link
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.

@erlend-aasland
Copy link
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 :)

@shihai1991
Copy link
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.

@erlend-aasland
Copy link
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 ! :)

@erlend-aasland
Copy link
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?

@shihai1991
Copy link
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 :)

@github-actions
Copy link

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

@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 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
Labels
awaiting review skip news stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants