[C API] Revisit usage of the PyCapsule C API with multi-phase initialization API

[vstinner]

BPO	41798
Nosy	@vstinner, @tiran, @encukou, @corona10, @yol, @shihai1991, @erlend-aasland
PRs	bpo-41798: Allocate the expat_CAPI on the heap memory. #24061 bpo-41798: Allocate the datetime.datetime_CAPI on the heap memory. #24096 bpo-41798: Allocate _decimal extension module CAPI on the heap #24117 bpo-41798: Allocate socket extension module CAPI on the heap #24126 bpo-41798: Allocate unicodedata CAPI on the heap #24128 bpo-41798: Allocate the _curses._C_API on the heap memory. #24186

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-01-23.12:49:36.720>
created_at = <Date 2020-09-16.14:07:49.185>
labels = ['expert-C-API', '3.10']
title = '[C API] Revisit usage of the PyCapsule C API with multi-phase initialization API'
updated_at = <Date 2021-01-25.09:46:36.596>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2021-01-25.09:46:36.596>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-01-23.12:49:36.720>
closer = 'shihai1991'
components = ['C API']
creation = <Date 2020-09-16.14:07:49.185>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 41798
keywords = ['patch']
message_count = 19.0
messages = ['376992', '376995', '379104', '379733', '384281', '384389', '384392', '384393', '384394', '384395', '384429', '384430', '384494', '384537', '384538', '385330', '385486', '385533', '385610']
nosy_count = 7.0
nosy_names = ['vstinner', 'christian.heimes', 'petr.viktorin', 'corona10', 'pkerling', 'shihai1991', 'erlendaasland']
pr_nums = ['24061', '24096', '24117', '24126', '24128', '24186']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue41798'
versions = ['Python 3.10']

[vstinner]

More and more extension modules are converted to the multi-phase initialization API (PEP-489) in bpo-1635741.

The problem is that the usage of the PyCapsule C API is not adapted in these extensions, to isolate well capsule objects from other module instances.

For example, the pyexpat extension uses "static struct PyExpat_CAPI capi;". It was fine when it was not possible to create more than one instance of the extension module. But with PR 22222 (currently under review), it becomes possible to have multiple extension module instances. Each module instance creates its own capsule object, but all capsule object points to the same unique "struct PyExpat_CAPI" instance.

For the specific case of the pyexpat in its current implementation, reusing the same "struct PyExpat_CAPI" instance is ok-ish, since the value is the same for all module instances. But this design sounds fragile.

It would be safer to allocate a "struct PyExpat_CAPI" on the heap memory in each module instance, and use a PyCapsule destructor function (3rd parameter of PyCapsule_New()).

The _ctypes does that:
---

        void *space = PyMem_Calloc(2, sizeof(int));
        if (space == NULL)
            return NULL;
        errobj = PyCapsule_New(space, CTYPES_CAPSULE_NAME_PYMEM, pymem_destructor);

---

with:
---

static void pymem_destructor(PyObject *ptr)
{
    void *p = PyCapsule_GetPointer(ptr, CTYPES_CAPSULE_NAME_PYMEM);
    if (p) {
        PyMem_Free(p);
    }
}

---

The PyCapsule API is used by multiple extension modules:

• _ctypes: allocate memory on the heap and uses a destructor to release it

• _curses: static variable, PyInit__curses() sets PyCurses_API[0] to &PyCursesWindow_Type (static type)

• _datetime: static variable, PyInit__datetime() creates a timezone object and stores it into CAPI.TimeZone_UTC

• _decimal: static variable

• _socket: static variable, PyInit__socket() sets PySocketModuleAPI.error to PyExc_OSError, and sets PySocketModuleAPI.timeout_error to _socket.timeout (a new exception object)

• pyexpat: static varaible

• unicodedata: static variable

• posix: nt._add_dll_directory() creates a PyCapsule using AddDllDirectory() result as a the pointer value
  The _datetime module overrides the

--

See also the PEP-630 "Isolating Extension Modules".

[vstinner]

unicodedata: static variable

Right now, it's ok-ish. Mohamed Koubaa is working on PR 22145 to pass a module state into internal functions, and so the PyCapsule pointer must be different in each module instance.

[encukou]

Also note that the capsule generally needs to hold references to the objects in exposes, and not rely on the module object to keep things alive. (Module objects with multi-phase init are not unique nor immortal.)
_ctypes is an exception, since it doesn't have ant PyObject*. But in most others the destructor should also contain some DECREFs.

[vstinner]

Also note that the capsule generally needs to hold references to the objects in exposes

Oh right, that's a great advice!

[vstinner]

New changeset 7c83eaa by Hai Shi in branch 'master':
bpo-41798: pyexpat: Allocate the expat_CAPI on the heap memory (GH-24061)
7c83eaa

[erlend-aasland]

Hai Shi, do you mind if I have a go at the socket module CAPI capsule?

[shihai1991]

Hai Shi, do you mind if I have a go at the socket module CAPI capsule?

Welcome to join this bpo, you can enhance those extension modules if you like :)

[erlend-aasland]

Welcome to join this bpo, you can enhance those extension modules if you like :)

Thanks :) I'll take a look at socket first.

[tiran]

PySocketModuleAPI.timeout_error now points to builtin TimeoutError exception.

I'm also in the process of removing PySocketModuleAPI from _ssl.c.

[erlend-aasland]

I'm also in the process of removing PySocketModuleAPI from _ssl.c.

That's the only user of the API, right? In that case, I'll target _decimal instead. Hai Shi is working on _curses and _datetime, AFAIK.

[erlend-aasland]

I'm also in the process of removing PySocketModuleAPI from _ssl.c.

BTW, do we still want to keep the socket CAPI for external users?