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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Feb 1, 2024
Merged

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

merged 1 commit into from
Feb 1, 2024

Conversation

charles-dyfis-net
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

@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.

@webknjaz webknjaz mentioned this pull request Jan 23, 2024
Closed
charles-dyfis-net added a commit to charles-dyfis-net/multidict that referenced this pull request Jan 24, 2024
@charles-dyfis-net
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 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
@charles-dyfis-net
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 charles-dyfis-net force-pushed the py312-no-private-interface branch from 1341d01 to 4241d0a Compare January 24, 2024 14:37
@charles-dyfis-net charles-dyfis-net force-pushed the py312-no-private-interface branch from 4241d0a to a24e0ba Compare January 24, 2024 14:40
@charles-dyfis-net
Copy link
Contributor Author

@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
Copy link
Member

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

@webknjaz
Copy link
Member

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 charles-dyfis-net force-pushed the py312-no-private-interface branch from a24e0ba to 45d6942 Compare January 24, 2024 15:12
@charles-dyfis-net
Copy link
Contributor Author

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

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 24, 2024
@charles-dyfis-net 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
@webknjaz
Copy link
Member

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

@webknjaz
Copy link
Member

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 charles-dyfis-net force-pushed the py312-no-private-interface branch from 8884206 to ed0cf16 Compare January 24, 2024 18:10
@charles-dyfis-net
Copy link
Contributor Author

@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
Copy link
Member

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

@webknjaz
Copy link
Member

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

@codecov Codecov
Copy link

codecov bot commented Jan 24, 2024 •
edited
Loading

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
Copy link
Contributor Author

charles-dyfis-net commented Jan 24, 2024 •
edited
Loading

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.

@chrisdickinson chrisdickinson mentioned this pull request Jan 25, 2024
Merged
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/909.bugfix.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/909.bugfix.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/909.bugfix.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/926.bugfix.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/909.bugfix.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/909.bugfix.rst Outdated Show resolved Hide resolved
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/909.bugfix.rst Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented Feb 1, 2024

@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!

@charles-dyfis-net charles-dyfis-net force-pushed the py312-no-private-interface branch 2 times, most recently from e0e07fc to f22c3d6 Compare February 1, 2024 02:08
webknjaz
webknjaz reviewed Feb 1, 2024
View reviewed changes
CHANGES/909.bugfix.rst Outdated Show resolved Hide resolved
@webknjaz

This comment was marked as resolved.

Sign in to view
@charles-dyfis-net charles-dyfis-net force-pushed the py312-no-private-interface branch 2 times, most recently from 5fb4dce to 39048ed Compare February 1, 2024 02:52
@charles-dyfis-net @webknjaz
Avoid private argument-parsing API on Python 3.12 also
5ac7591 
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>
@charles-dyfis-net charles-dyfis-net force-pushed the py312-no-private-interface branch from 39048ed to 5ac7591 Compare February 1, 2024 02:52
webknjaz
webknjaz approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Thank you 🙏

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

@webknjaz webknjaz changed the title Extend #909 to 3.12 as well to address #926 Fix SEGFAULTs under Python 3.12 by using public C-API Feb 1, 2024
@webknjaz webknjaz merged commit 983411b into aio-libs:master Feb 1, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webknjaz webknjaz webknjaz approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees
No one assigned
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to pip install multidict with gcc-14
2 participants
@charles-dyfis-net @webknjaz