New MultiDict C-API functions And Added Documentation #1185

Vizonex · 2025-06-21T01:51:06Z

What do these changes do?

First of all, thank you for giving something better to work with I really appreciate it.
This Adds a few new api calls mainly belonging to Multidict's api I had a proposed idea for an iterator but I think I might delete it and call it a day. I wasn't lazy at all about it in fact it encouraged me to keep going. I was pretty creative about exposing some functions such as MultiDicts updating each other or using Sequence and Mapping so that they could be utilized as shortcuts if needed.

Are there changes in behavior for the user?

Only the Good stuff is coming, I even wrote doxygen comments into each functions so that users would have a much better time using the C-API. Writing the pytest stuff is on my todo list I haven't done it yet but maybe sometime tomorrow I will finish this.

Related issue number

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes

…ent compile collisions with other libraries

Vizonex · 2025-06-21T01:57:54Z

Probably a good thing as well that I chose this route instead of the one I was going down I didn't feel as confused with what I was doing this time.

…tigate a little bit more deeply...

for more information, see https://pre-commit.ci

Vizonex · 2025-06-21T06:16:31Z

After I sleep I will need to investigate This error I get when running pytest

(multidict) PS ...> pytest tests/test_capi.py
==================================================================== test session starts =====================================================================
platform win32 -- Python 3.10.18, pytest-8.4.0, pluggy-1.6.0
codspeed: 3.2.0 (disabled, mode: walltime, timer_resolution: 100.0ns)
rootdir: [...]
configfile: pytest.ini
plugins: cov-6.1.0, codspeed-3.2.0
collected 3 items

tests\test_capi.py Windows fatal exception: access violation

The other 2 tests work just fine except for md_new(0).

Vizonex · 2025-06-21T14:07:03Z

I woke up time to finish this.

Vizonex · 2025-06-21T14:12:21Z

@asvetlov I solved the problem with MultiDict_New Adding a reference count by 1 stops the segfault.

Vizonex · 2025-06-21T14:14:39Z

When I'm done testing the C stuff I will redo the Cython API in the same format as testcapi to keep things organized.

for more information, see https://pre-commit.ci

Vizonex · 2025-06-21T15:55:28Z

there seems to be some bugs with MultiDict_Get where instead of returning None it returns NULL and says it didn't throw an exception either. Something confusing might be happening in the background.

asvetlov

Looks good, thanks!
Please let me carefully re-read the source again a little later before the merging.

multidict/multidict_api.h

asvetlov · 2025-06-21T15:56:18Z

multidict/__init__.py

+    cython code
+    """
+
+    return str(pathlib.Path(__file__).parent)


We have two options here.

Drop the method, document import multidict, include_path=multidict.__path__ as I did in testcapi/setup.py

Keep the function but return bare __path__; pathlib usage is redundant here

@asvetlov There was an issue when I tried compiling it but it may have been a configuration problem on my end so met let me go back and revert this change.

for more information, see https://pre-commit.ci

Vizonex · 2025-06-21T17:13:04Z

@asvetlov I'm going to finish writing all the tests should we worry about patching up the bugs now or later?

for more information, see https://pre-commit.ci

Vizonex · 2025-06-21T17:40:40Z

Only one thing is left on the table now will be debugging.

SKIPPED [1] tests\test_capi.py:69: SystemError: <built-in function md_get> returned NULL without setting an exception
FAILED tests/test_capi.py::test_md_get_all - TypeError: self should be a MultiDict instance
FAILED tests/test_capi.py::test_md_popone_raise - Failed: DID NOT RAISE <class 'KeyError'>
FAILED tests/test_capi.py::test_md_popall_key_error - Failed: DID NOT RAISE <class 'KeyError'>
FAILED tests/test_capi.py::test_md_update_from_md - TypeError: md_replace should be called with md, key and value
FAILED tests/test_capi.py::test_md_update_from_dict - TypeError: md_update_from_md should be called with md, other, and update
FAILED tests/test_capi.py::test_md_update_from_seq - TypeError: md_update_from_md should be called with md, other, and update
FAILED tests/test_capi.py::test_md_equals_1 - SystemError: <built-in function md_equals> returned NULL without setting an exception

for more information, see https://pre-commit.ci

Vizonex · 2025-06-21T21:30:00Z

@asvetlov Doesn't seem like I can merge this is there anything I'm missing that you would suggest I add before I start making more things later so that I don't overburden anyone?

Vizonex · 2025-06-22T02:02:20Z

While I wait let me fill out the documentation for these new functions we added to the capi since I figured out how to do so.
https://www.sphinx-doc.org/en/master/usage/domains/c.html

asvetlov

Hey, I've left a bunch of comments.

Let's process them.

asvetlov · 2025-06-22T10:30:22Z

multidict/_multilib/capsule.h

+static int
+MultiDict_Clear(void* state_, PyObject* self)
+{
+    // TODO: Macro for repeated steps being done?


Maybe the macro is a good idea, please feel free to try

I will I moved to #1190 but I am making sure to read over all your suggestions real quick before I move on...

asvetlov · 2025-06-22T10:34:55Z

multidict/_multilib/capsule.h

+        return -1;
+    }
+    if (md_del((MultiDictObject*)self, key) < 0) {
+        PyErr_SetObject(PyExc_KeyError, key);


This line converts all errors (for example, TypeError if passed key is not a string) to KeyError; it is not good.

Better to just return a result of md_del() as is; md_del() already sets KeyError if the key is not found.

asvetlov · 2025-06-22T10:43:00Z

multidict/_multilib/capsule.h


-static PyTypeObject *
-MultiDict_GetType(void *state_)
+/// @brief Gets the multidict type object


Regarding Doxigen comments -- I'm not sure if we need it.

We should either set up a Doxygen docs generator, as we already do for Sphinx docs, or we should drop these comments.

Without an automatic generator/checker, the comments eventually will go out of sync with the code itself; misleading comments are harmful.

Taking into account that we use Sphinx already, maybe better to document C API in sphinx also?

Aha, I see the comment that you already started Sphinx docs. In this case, Doxigen comments don't make sense.

Aha, I see the comment that you already started Sphinx docs. In this case, Doxigen comments don't make sense.

I documented everything as I was writing everything so that moving things around would be less painful to move but I think we still need a readable format to have in the api header file incase some user decides to hover over the function and wonder what it's doing immediately. Msvc's C/C++ Extension seems to autogenerate these comments and allows me to see the definitions when I hover my mouse of them. However, I am willing to remove delete or reformat them all since I'm done moving the current sphinx documentation over.

asvetlov · 2025-06-22T10:51:26Z

multidict/_multilib/capsule.h

+        return NULL;
+    }
+    PyObject* ret = NULL;
+    if (md_get_one((MultiDictObject*)self, key, &ret) < 0) {


md_get_one() is designed to quickly figure out if the key exists or not without analyzing Python exception.

PyErr_ExceptionMatches() + PyErr_Clear() is much slower than if (ret == NULL).

I think MultiDict_GetOne() should follow PyDict_GetItemRef.
As I see, now md_get_one returns -1 if an exception was raised, 0 otherwise. Maybe better return 0 if not found, 1 if found. Let's do it in a separate PR against the master branch; the fix is not directly related to CAPI itself.

As I see, now md_get_one returns -1 if an exception was raised, 0 otherwise. Maybe better return 0 if not found, 1 if found. Let's do it in a separate PR against the master branch; the fix is not directly related to CAPI itself.

@asvetlov Sure, I would be willing to work on that when I'm done making fixes into this branch shouldn't be too difficult to port something like that into #1190 and another branch

asvetlov · 2025-06-22T10:55:07Z

multidict/_multilib/capsule.h

+/// @return returns a default value on success,  NULL is returned and raises
+/// `TypeError` on failure
+static PyObject*
+MultiDict_Get(void* state_, PyObject* self, PyObject* key)


Do we need MultiDict_Get() if GetOne()/GetAll() also provided?
In python API, MultiDict.get() exists because multidict implements collections.abc.Mapping interface.
In C, we do not have such a contract.
If the function doesn't exist, nobody should learn how the function works.

asvetlov · 2025-06-22T11:51:42Z

multidict/_multilib/capsule.h

+        _invalid_type();
+        return -1;
+    }
+    if (PyMapping_Check(other)) {


MultiDict_Check should go first. PyMapping_Check succeeds for multidict objects, which leads to a correct but slow path.

Alright let me make sure it has that in #1190 then...

asvetlov · 2025-06-22T11:58:28Z

multidict/_multilib/capsule.h

+/// @return 1 if true, 0 if false, -1 if failue occured follwed by raising a
+/// TypeError
+static int
+MultiDict_Equals(void* state_, PyObject* self, PyObject* other)


I doubt if the function should be exposed; standard Python containers (dict, list, tuple, set) have no comparison functions, PyObject_RichCompare() does its job well.
The only exception is str, which has a rich set of comparison functions to work with objects, ascii and utf8 strings.

asvetlov · 2025-06-22T12:02:12Z

multidict/multidict_api.h

    */

-    void *state;
+    void* state;


Cython branch will be setup once the C-API is done but I will keep it under the same format as the C-API and the module, the setup will be configured to same way as testcapi only this time I will name it testcythonapi.
Minor: testcapi could have two modules, one for C api test helpers and another one for Cython. Maybe we can even provide the same API by both wrappers to run the same api tests for both implementations, like we run the same tests suite for pure python and C extensions.

asvetlov · 2025-06-22T12:05:08Z

testcapi/testcapi/_api.c

 {
    mod_state *state = get_mod_state(self);
-    return MultiDict_New(state->capi, 0);
+    return Py_NewRef(MultiDict_New(state->capi, 0));


This is suspicious.
MultiDict_New should return a new reference; double incref is not needed.

Removing Py_NewRef here triggers Segfault when running in pytest tests\test_capi.py

tests\test_capi.py Windows fatal exception: access violation

Perhaps maybe I should move NewRef into Multidict_New instead?

Or Maybe Py_INCREF?

asvetlov · 2025-06-22T12:08:25Z

testcapi/testcapi/_api.c

+
+#define RETURN_NULL_OR_NEWREF(ITEM) \
+    PyObject* REF = ITEM; \
+    return (REF != NULL) ? Py_NewRef(REF) : NULL


Why newref here? It looks like a memory leak generation.

My reasoning behind this was that not doing so would cause the program to segfault.

asvetlov · 2025-06-22T12:18:16Z

Sorry, I've merged the PR by accident; I'll revert the merge.
PR is not ready yet.

asvetlov · 2025-06-22T13:18:58Z

I've reverted the merge into aio-libs:capi but I have no idea how to change the status from 'merged' back to 'in-progress', sorry.

Vizonex · 2025-06-22T18:18:14Z

I've reverted the merge into aio-libs:capi but I have no idea how to change the status from 'merged' back to 'in-progress', sorry.

@asvetlov It's fine I'll try continuing where I left off in a new branch by just re-forking everything off again.

Vizonex added 8 commits June 20, 2025 15:21

make capsule api functions less generic by adding a namespace to prev…

09b466b

…ent compile collisions with other libraries

missed a function let me fix it real fast...

0b08ba3

lets add some documentation comments to these functions

baf4e6a

time to add a few more functions, I'm adding more shortly...

8356ff2

Fill out more functions and concept ideas

c884dcb

all the Multidict Object API Next will be istr...

dc2bcc7

reformat with clang-format

904fb82

add more functions to testcapi :)

0ebbdf1

Vizonex requested a review from asvetlov as a code owner June 21, 2025 01:51

add contribution to changes folder

ecd898c

Vizonex requested a review from webknjaz as a code owner June 21, 2025 01:54

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jun 21, 2025

Vizonex and others added 6 commits June 20, 2025 23:21

Revert namespace idea unsure if it actually will cause compiling issues

efa70f5

Revert namespace idea unsure if it actually will cause compiling issues

9036575

I kept my good old backups

17b560d

fix compiling with previously proposed api

4e08873

discovered that md_new(...) is causing segfaults, gonna need to inves…

c04d584

…tigate a little bit more deeply...

[pre-commit.ci] auto fixes from pre-commit.com hooks

6c80da6

for more information, see https://pre-commit.ci

Vizonex and others added 2 commits June 21, 2025 09:15

Adding a reference count by 1 solves the segfault

721b593

[pre-commit.ci] auto fixes from pre-commit.com hooks

670fe1d

for more information, see https://pre-commit.ci

Vizonex changed the title ~~New MultiDict C-API functions~~ New MultiDict C-API functions And Added Documentation Jun 21, 2025

Vizonex added 4 commits June 21, 2025 09:31

Now that we know a bit more about refs let's change a few of these

233545f

Ensure MultiDict_Del throws exception when key not found

d6c10a7

add test_del

3d2e83a

Merge branch 'capi' of https://github.com/Vizonex/multidict into capi

1a8e629

Vizonex and others added 3 commits June 21, 2025 10:50

run reformatter

98f0144

add few more tests if I haven't yet done so

b7208da

[pre-commit.ci] auto fixes from pre-commit.com hooks

1e97e03

for more information, see https://pre-commit.ci

asvetlov reviewed Jun 21, 2025

View reviewed changes

Vizonex and others added 6 commits June 21, 2025 10:58

use precommit suggestions

756e331

revert

a9ec875

whoops I forgot to remove

b4239ac

mypy fixes

2eb9032

[pre-commit.ci] auto fixes from pre-commit.com hooks

3bb5a74

for more information, see https://pre-commit.ci

Merge branch 'capi' into capi

04c2f83

Vizonex and others added 2 commits June 21, 2025 12:38

all tests added we can begin debugging soon...

3b03439

[pre-commit.ci] auto fixes from pre-commit.com hooks

164be59

for more information, see https://pre-commit.ci

Vizonex added 2 commits June 21, 2025 14:55

ALL OF THEM WORK!!!!

da32a5e

Merge branch 'capi' of https://github.com/Vizonex/multidict into capi

d691d6d

Vizonex requested a review from asvetlov June 21, 2025 19:58

pre-commit-ci bot and others added 3 commits June 21, 2025 19:58

[pre-commit.ci] auto fixes from pre-commit.com hooks

1a56fc5

for more information, see https://pre-commit.ci

reformat incase I haven't already done so

0c73888

Merge branch 'capi' of https://github.com/Vizonex/multidict into capi

8f94d17

asvetlov reviewed Jun 22, 2025

View reviewed changes

asvetlov merged commit 8f94d17 into aio-libs:capi Jun 22, 2025
5 checks passed

asvetlov mentioned this pull request Jun 22, 2025

Slightly rewrite internal C API to reflect the modern dict API #1187

Merged

Vizonex mentioned this pull request Jun 22, 2025

Add More Multidict Functions C-API #1190

Open

3 tasks

Uh oh!

Uh oh!

New MultiDict C-API functions And Added Documentation #1185

New MultiDict C-API functions And Added Documentation #1185

Uh oh!

Conversation

Vizonex commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 21, 2025

Uh oh!

Vizonex commented Jun 22, 2025

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vizonex Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Vizonex commented Jun 21, 2025 •

edited

Loading

Vizonex Jun 22, 2025 •

edited

Loading