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

feat: implement PEP489 (multi-phase module init, heap-allocated types) for Python >= 3.11 #204

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

tseaver
Copy link
Member

@tseaver tseaver commented Jun 1, 2024

Older Python versions continue to use static type init and static classes, although static state has been moved into the module state unconditionally.

I realize these changes are likely to be disruptive, in particular, for BTrees, which I plan to handle next.

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tseaver Sorry I lost track of this one. I haven't reviewed the whole thing yet; so far I looked at the headers and _timestamp.c

src/persistent/_timestamp.c Show resolved Hide resolved
src/persistent/_timestamp.c Outdated Show resolved Hide resolved
src/persistent/_timestamp.c Outdated Show resolved Hide resolved
@davisagli
Copy link
Member

I tried running tox -e py311 using a Python built with --with-assertions. It fails to compile:

      building 'persistent.cPickleCache' extension
      clang -Wsign-compare -Wunreachable-code -g -fwrapv -O3 -Wall -I/Users/davisagli/Plone/persistent/.tox/py311/include -I/Users/davisagli/.pyenv/versions/3.11.8/include/python3.11 -c src/persistent/cPickleCache.c -o build/temp.macosx-13.6-arm64-cpython-311/src/persistent/cPickleCache.o
      src/persistent/cPickleCache.c:640:27: error: no member named 'ob_refcnt' in 'cPersistentObject'
          assert(dead_pers_obj->ob_refcnt == 0);
                 ~~~~~~~~~~~~~  ^
      /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/assert.h:99:25: note: expanded from macro 'assert'
          (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __ASSERT_FILE_NAME, __LINE__, #e) : (void)0)
                              ^
      src/persistent/cPickleCache.c:650:27: error: no member named 'ob_refcnt' in 'cPersistentObject'
          assert(dead_pers_obj->ob_refcnt == 1);
                 ~~~~~~~~~~~~~  ^
      /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/assert.h:99:25: note: expanded from macro 'assert'
          (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __ASSERT_FILE_NAME, __LINE__, #e) : (void)0)
                              ^
      src/persistent/cPickleCache.c:679:27: error: no member named 'ob_refcnt' in 'cPersistentObject'
          assert(dead_pers_obj->ob_refcnt == 1);
                 ~~~~~~~~~~~~~  ^
      /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/assert.h:99:25: note: expanded from macro 'assert'
          (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __ASSERT_FILE_NAME, __LINE__, #e) : (void)0)
                              ^
      3 errors generated.
      error: command '/usr/bin/clang' failed with exit code 1

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of cPersistence.c

src/persistent/cPersistence.c Outdated Show resolved Hide resolved
src/persistent/cPersistence.c Show resolved Hide resolved
src/persistent/cPersistence.c Outdated Show resolved Hide resolved
src/persistent/cPersistence.c Show resolved Hide resolved
@tseaver
Copy link
Member Author

tseaver commented Aug 28, 2024

@davisagli
607cb29 fixes the compile-time errors under a Python built using --with-assert -- there are other issues at runtime I haven't chased down yet.

- Remove 'tp_dealloc' / 'tp_clear' slots (there are no allocated members).

- Use 'timestamp_type->tp_alloc' rather than 'PyObject_New'.
Creating instances of 'type' or 'tuple' via 'PyType_GenericNew' leaves
them in states which trigger assertions in their 'tp_dealloc' slots.

These assertions are normally silent, but trigger when running under a
Python built using '--with-debug' or '--with-assertions'.
@tseaver tseaver requested a review from davisagli August 28, 2024 20:40
@tseaver
Copy link
Member Author

tseaver commented Aug 28, 2024

@davisagli The last four commits clear up the remaining issues: tests now pass without segfaults under both "normal" Python >= 3.11 and under a --with-debug / --with-asserts Python.

That struct is actually just a borrowed pointer to the struct held in the
module state of the 'cPersistence' extension module.  Visiting / clearing
it from withing 'cPickleCache' is inappropriate, and may lead to
segfaults during program exit.
@tseaver
Copy link
Member Author

tseaver commented Aug 28, 2024

@davisagli re c8b34ca:

I backed out the Py_INCREF (and related Py_VISIT and Py_CLEAR) in cPickleCache.c for the capi_struct->pertype member: the capi_struct is literally just a borrowed pointer to the structure which is part of the module state in cPersistence.c, and cPickleCache doesn't have the responsibility to manage it. The Py_CLEAR, in particular, could cause a segfault during normal interpreter shutdown.

Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mentioned that this will be disruptive. Should we bump the major version as part of merging this?

@tseaver
Copy link
Member Author

tseaver commented Aug 29, 2024

@davisagli

You mentioned that this will be disruptive. Should we bump the major version as part of merging this?

I'm not sure. Likely should try installing the wheel built from this branch into tox environments for the BTrees master branch, and see if anything breaks. AFAIK, BTrees is the only C-level consumer of the headers in persistent. Oops, no, the Zope2 Persistence lib uses it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants