Turn rapidjson.Decoder into heap type - #227

This PR is a first step of work on the issue 226. Other types declared by the module must be converted to heap types as it's recommended in the cpython docs

Because they are immutable and process-global, static types cannot access “their” module state. If any method of such a type requires access to module state, the type must be converted to a heap-allocated type, or heap type for short. These correspond more closely to classes created by Python’s class statement.

For new modules, using heap types by default is a good rule of thumb.

The main purpose of having this small step as individual PR is getting early feedback on coding style issues or getting other comments which should be taken into account on further work.

Hi @VestniK, I created the sub-interpreters branch where I rebased this PR on top of current master (where I dropped support for Python <3.9), plus @espressolee change.

I saw that in your master fork you addressed the other types: I wonder if you are still interested in completing the work, rebasing remaining changes on top of my sub-interpreters branch.

Thanks, and sorry for this taking so looong...

Hi, I took the liberty of cherry-pick-with-small-tweaks other commits from @VestniK master, to complete migration to heap types.

It's somewhat difficult for me to judge the remaining commits:

• is it really necessary to introduce that cmake-conan?
• can the subinterpreter_tests directory be moved to tests/subinterpreters?
• is the latest TMP commit valuable?

It will take some time to me to answer these questions, so any help will be really appreciated.

I checked the three remaining commits and also built the current sub-interpreters head ec591c83e995b1b639e578349914d26dec6ad804 on CPython 3.14.6.

Short answers:

1. I do not think cmake-conan is necessary. The test commit introduces a second build/dependency stack (a git submodule, Conan, a separately pinned CPython 3.12.7, RapidJSON 1.1.0, Catch2, and CMake 4.0) only for this test. That can drift from the extension and RapidJSON revision actually built by setup.py/CI. The embedded test idea is useful, but it can use the checked-out RapidJSON headers and the running Python's normal embed development target, with either a very small assertion-based executable or a Python-level _interpreters regression on versions where that API is available. I would leave the Conan provider out.

2. Yes, subinterpreter_tests can be moved to tests/subinterpreters. Nothing in the test code depends on its current top-level location; only the build path needs updating. That location also makes its role much clearer.

3. The goal of TMP is necessary, but the exact commit is not correct/useful as-is. The current maintainer branch advertises Py_MOD_PER_INTERPRETER_GIL_SUPPORTED while still using process-global static Cache cache. I can make the resulting cross-interpreter corruption deterministic:

   • create isolated own-GIL interpreters A and B;
   • import rapidjson, decimal, and uuid in A and successfully serialize A's Decimal/UUID objects;
   • import rapidjson in B;
   • serialize A's Decimal again in A.

   The final operation returns TypeError: Decimal('1.5') is not JSON serializable, because B's module_exec() overwrote the process-global cached type objects. The normal exact-head suite still passes (929 passed, 17 skipped, 2 xfailed), so an import-only subinterpreter test does not catch this.

   I then built exact fcdba813c4b5efa27bf42e7a416cf4c69582ca8f. The compiler reports unused variable 'cache2', and the same two-interpreter probe fails in the same way. That commit placement-news a Cache in module state, but all live paths still read and populate the global cache; it never uses cache2. Its lifecycle hooks also need another pass: mod_clear() only calls the trivial C++ destructor and does not Py_CLEAR the owned references, mod_free() calls mod_clear() again, and Types::traverse() omits rawjson.

My recommendation is therefore:

• keep the heap-type conversions;
• do not add cmake-conan;
• move the tests under tests/subinterpreters;
• hold/remove the Py_mod_multiple_interpreters slot until every cache access is actually routed through PyModule_GetState() for module functions and PyType_GetModuleState() (or an equivalent module association) for heap-type methods;
• extend the regression beyond concurrent import: perform cache-dependent operations in both interpreters after both imports, then destroy one interpreter and use the survivor again.

So the TMP problem is load-bearing, but the TMP implementation should be rewritten rather than cherry-picked.

Thank you. I will try to replace that static cache with an equivalent slot in the module's state.

I will try to replace that static cache with an equivalent slot in the module's state.

I have a feeling that I'm reaching the edge of my (severely limited) C++-fu ... 🫤

I pushed my attempt in a temporary branch: lelit@d44728f

The show stopper is this: the validator_new() function needs to module's state, and to that needs to reference the static PyModuleDef module defined at the end of the source. As is, the compiler rightfully reject it:

./rapidjson.cpp: In function ‘PyObject* validator_new(PyTypeObject*, PyObject*, PyObject*)’:
./rapidjson.cpp:3880:51: error: ‘module’ was not declared in this scope; did you mean ‘modfl’?
 3880 |     PyObject* self = PyType_GetModuleByDef(type, &module);
      |                                                   ^~~~~~

but it does not seem possible to add a /forward declaration/ to a static struct in C++...

I could change the /linkage/ of the module structure, renaming it to, say, rj_module but... am I on the right track here?

Needs to sleep on this!

You're on a workable track. The reason the forward declaration won't go in: C++ has no non-defining namespace-scope declaration spelled static, so static PyModuleDef module; is already a definition and the later initializer is a redefinition — and extern first then static is rejected outright.

If you'd rather not rename, an extern declaration inside an anonymous namespace is a real forward declaration and keeps internal linkage:

namespace { extern PyModuleDef module; }
…
namespace { PyModuleDef module = { … }; }

That and your rj_module both build. The only difference I measured is that in my macOS build the external-linkage version additionally exported rj_module from the extension; the anonymous-namespace one did not.

The thing that will actually bite is unrelated to any of that: the public PyType_GetModuleByDef arrived in 3.11. Compiling that call against 3.10 headers gives

error: use of undeclared identifier 'PyType_GetModuleByDef'; did you mean '_PyType_GetModuleByDef'?

3.10 exposes only the private _PyType_GetModuleByDef, which is internal and outside the stable ABI, so I would not lean on it. setup.py still lists 3.10 and, as configured, cibuildwheel looks like it includes cp310 — CIBW_SKIP excludes only cp39* — so this is likely to fail the next time the wheel job runs. The test matrix is 3.13 only, so CI wouldn't catch it first. Dropping 3.10 looks cleaner to me than carrying a separate 3.10 path, but that's your call.

Thanks a lot! Given that Python 3.10 is going EOL next month, I'd be inclined to dropping it... will think about it a bit more.