Fix SEGFAULTs under Python 3.12 by using public C-API #929

charles-dyfis-net · 2024-01-20T05:16:44Z

What do these changes do?

Fix build issues (observed personally with clang 16.0.6 as wrapped by nixpkgs when building multidict via poetry2nix, and reported by @psss to also happen on gcc-14) related to changes to the _PyArg_Parser structure by moving away from this internal interface to use a slower, fully-supported interface instead.

Are there changes in behavior for the user?

This change has a performance penalty.

Related issue number

Trivial modification to the work done in #909.
Fixes #926.

Checklist

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

charles-dyfis-net · 2024-01-20T05:18:44Z

I'm extremely confident in my ability to build a reproducer for this problem for systems running Nix on Darwin, and moderately confident in my ability to build a reproducer for systems running Nix on Linux. I have not at all investigated reproduction of the issue without Nix, so I'm not really sure what a test suite addition would look like.

webknjaz · 2024-01-23T16:30:26Z

@charles-dyfis-net plz update the change note from #909 and add a symlink with this PR number in it.

Meanwhile, I want to figure out why it works without this change.

Implemented as an extension of aio-libs#909, which disabled METH_FASTCALL on Python 3.13 and later. Fixes aio-libs#926.

charles-dyfis-net · 2024-01-24T14:42:41Z

@charles-dyfis-net plz update the change note from #909 and add a symlink with this PR number in it.

Implemented, I thought -- a24e0ba appends to CHANGES/909.bugfix.rst -- but the "timeline protection" job appears to still be failing. Should this be a new CHANGES/ entry instead?

Meanwhile, I want to figure out why it works without this change.

I'll be curious what you learn.

webknjaz · 2024-01-24T14:45:55Z

The not doesn't treat edits to old fragments the same. But if you add a symlink, it should notice that.

webknjaz · 2024-01-24T14:49:33Z

Could you try adding a test with the suggestion from #926 (comment) and check if it fails w/o this patch and succeeds with it? Also, maybe test with the code pre #909.
I don't want to merge this PR without any regression tests. The previous experience with bad changes shows that it's not viable.

charles-dyfis-net · 2024-01-24T15:12:30Z

Could you try adding a test with the suggestion from #926 (comment) and check if it fails w/o this patch and succeeds with it? Also, maybe test with the code pre #909. I don't want to merge this PR without any regression tests. The previous experience with bad changes shows that it's not viable.

That test is in fact helpful!

Without this change, it results in a segfault:

Fatal Python error: Segmentation fault

Current thread 0x00000001da52d000 (most recent call first):
  File "/Users/chaduffy/VC/multidict/tests/test_multidict.py", line 214 in test_call_with_kwargs
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/python.py", line 194 in pytest_pyfunc_call
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/python.py", line 1792 in runtest
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/runner.py", line 169 in pytest_runtest_call
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/runner.py", line 262 in <lambda>
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/runner.py", line 341 in from_call
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/runner.py", line 261 in call_runtest_hook
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/runner.py", line 222 in call_and_report
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/runner.py", line 133 in runtestprotocol
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/runner.py", line 114 in pytest_runtest_protocol
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/main.py", line 350 in pytest_runtestloop
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/main.py", line 325 in _main
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/main.py", line 271 in wrap_session
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/main.py", line 318 in pytest_cmdline_main
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_callers.py", line 102 in _multicall
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_manager.py", line 119 in _hookexec
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/pluggy/_hooks.py", line 501 in __call__
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/config/__init__.py", line 169 in main
  File "/Users/chaduffy/VC/multidict/test.venv/lib/python3.12/site-packages/_pytest/config/__init__.py", line 192 in console_main
  File "/Users/chaduffy/VC/multidict/test.venv/bin/pytest", line 8 in <module>

Extension modules: multidict._multidict (total: 1)
Segmentation fault: 11

webknjaz · 2024-01-24T16:39:07Z

@charles-dyfis-net thanks! Could you also run that test against multidict < 6? I'm curious if it ever worked...

webknjaz · 2024-01-24T17:53:58Z

FYI there's a text file with allowed words in the docs dir. You can add "GCC" there to make the linting succeed.

charles-dyfis-net · 2024-01-24T18:14:36Z

@charles-dyfis-net thanks! Could you also run that test against multidict < 6? I'm curious if it ever worked...

The test passes on 5.2.0, so it looks like this is a 6.x-only bug.

webknjaz · 2024-01-24T19:24:38Z

And is it correct that it succeeds with Python 3.9 and multidict 6.0.0?

webknjaz · 2024-01-24T19:25:31Z

It'd be useful to mention in the change note when exactly the regression happened and in which envs.

codecov · 2024-01-24T19:36:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (770445a) 96.74% compared to head (5ac7591) 96.74%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #929   +/-   ##
=======================================
  Coverage   96.74%   96.74%           
=======================================
  Files          19       19           
  Lines        1965     1968    +3     
  Branches      188      188           
=======================================
+ Hits         1901     1904    +3     
  Misses         40       40           
  Partials       24       24

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

charles-dyfis-net · 2024-01-24T21:00:44Z

And is it correct that it succeeds with Python 3.9 and multidict 6.0.0?

6.0.0 does not fail the test with Python 3.9 (and can't be built at all against 3.12, at least on darwin with clang 15.0.0 or 16.0.6).

My understanding is that this is a 3.12-specific bug; I suspect, but have not validated, that it was probably present from the point in the 6.x series where 3.12 support was first added.

CHANGES/909.bugfix.rst

CHANGES/926.bugfix.rst

CHANGES/909.bugfix.rst

webknjaz · 2024-02-01T01:32:11Z

@charles-dyfis-net so I made a few final changelog-related suggestions. Assuming you want to squash them into your commit yourself, let me know and I'll merge this!

CHANGES/909.bugfix.rst

Implemented as an extension of aio-libs#909, which made this change on Python 3.13 and later. Fixes aio-libs#926 (build failure observed with clang-16.0.6 and gcc-14). Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>

webknjaz

Thank you 🙏

P.S. Codecov says this PR also increases coverage for 0.01% 😃

charles-dyfis-net requested a review from asvetlov as a code owner January 20, 2024 05:16

webknjaz mentioned this pull request Jan 23, 2024

Unable to pip install multidict with gcc-14 #926

Closed

charles-dyfis-net added a commit to charles-dyfis-net/multidict that referenced this pull request Jan 24, 2024

Avoid METH_FASTCALL on Python 3.12 (PR aio-libs#929)

1341d01

Implemented as an extension of aio-libs#909, which disabled METH_FASTCALL on Python 3.13 and later. Fixes aio-libs#926.

charles-dyfis-net force-pushed the py312-no-private-interface branch from ad11386 to 1341d01 Compare January 24, 2024 14:33

charles-dyfis-net added a commit to charles-dyfis-net/multidict that referenced this pull request Jan 24, 2024

Avoid METH_FASTCALL on Python 3.12 (PR aio-libs#929)

4241d0a

Implemented as an extension of aio-libs#909, which disabled METH_FASTCALL on Python 3.13 and later. Fixes aio-libs#926.

charles-dyfis-net force-pushed the py312-no-private-interface branch from 1341d01 to 4241d0a Compare January 24, 2024 14:37

charles-dyfis-net requested a review from webknjaz as a code owner January 24, 2024 14:37

charles-dyfis-net force-pushed the py312-no-private-interface branch from 4241d0a to a24e0ba Compare January 24, 2024 14:40

charles-dyfis-net force-pushed the py312-no-private-interface branch from a24e0ba to 45d6942 Compare January 24, 2024 15:12

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 24, 2024

charles-dyfis-net force-pushed the py312-no-private-interface branch 2 times, most recently from 5176ee3 to 8884206 Compare January 24, 2024 16:21

charles-dyfis-net force-pushed the py312-no-private-interface branch from 8884206 to ed0cf16 Compare January 24, 2024 18:10

chrisdickinson mentioned this pull request Jan 25, 2024

Upstreaming a number of deps dicej/wasi-wheels#1

Merged