fix: handle null `py::handle` and add tests for `py::scoped_critical_section` #5706

XuehaiPan · 2025-05-30T09:42:45Z

Description

A minor improvement to check if the handle is null.

Suggested changelog entry:

handle null py::handle for py::scoped_crititcal_section
Add tests for py::scoped_crititcal_section

Skylion007 · 2025-05-30T15:33:52Z

Add a note to: https://peps.python.org/pep-0703/#python-critical-sections

rwgk · 2025-05-30T15:40:16Z

@XuehaiPan Could you please add tests?

henryiii · 2025-05-30T22:31:25Z

What did you have in mind for tests? It's not that easy to test that this is or is not locking. You could just make sure it was callable with nullptr (since that does nothing), I suppose?

rwgk · 2025-05-31T16:56:23Z

What did you have in mind for tests? It's not that easy to test that this is or is not locking. You could just make sure it was callable with nullptr (since that does nothing), I suppose?

Sorry I somehow missed this question before.

Just covering all situations with nullptr. Does that make sense?

Basic code coverage.

Currently we have only this:

                py::scoped_critical_section lock(locals);

rwgk · 2025-05-31T16:58:41Z

I forgot to mention: I'm wondering, how is py::scoped_critical_section(handle()) useful? It'd be great to explain that somewhere, e.g. here in the PR description.

henryiii · 2025-06-01T04:00:21Z

Looks like C++20+ has a problem.

henryiii · 2025-06-03T06:01:52Z

Do you think you can solve the segfaults for C++20 and 23?

XuehaiPan · 2025-06-03T17:03:25Z

The test failure on macOS with Python 3.14t seems unrelated and I cannot reproduce locally.

rwgk · 2025-06-03T23:38:59Z

Drive-by comment.

The new test is much more comprehensive than I was hoping! I'll need to find some time to look carefully. (Hopefully later today.)

henryiii · 2025-06-04T03:29:59Z

I don't think it's valid in the free-threading build to call gc.collect and expect things to be collected. Collections happen as part of "stop-the-world" events, which that's not going to trigger, as it needs to sync the dual ref-counts. I think that's why the 3.14t test is failing.

Generally gc-based tests are not very good, there really aren't strong language-level guarantees about things getting cleaned up, it's implementation based.

… guard against name clashes).

…combinations()

rwgk · 2025-06-04T05:09:35Z

tests/test_scoped_critical_section.cpp

+namespace test_scoped_critical_section_ns {
+
+void test_one_nullptr() { py::scoped_critical_section lock{py::handle{}}; }
+
+void test_two_nullptrs() { py::scoped_critical_section lock{py::handle{}, py::handle{}}; }
+
+void test_first_nullptr() {
+    py::dict d;
+    py::scoped_critical_section lock{py::handle{}, d};
+}
+
+void test_second_nullptr() {
+    py::dict d;
+    py::scoped_critical_section lock{d, py::handle{}};
+}


When I was suggesting to add tests, all I had in mind was something simple like this.

The additional tests look interesting though.

Could you please add comments to explain what the tests are for? (I'd ask my favorite LLM for suggestions, usually it's really quick that way.)

include/pybind11/critical_section.h

tests/test_methods_and_attributes.py

tests/test_pickling.py

rwgk · 2025-06-04T05:39:23Z

This crossed my mind:

If it's troublesome to get the new comprehensive tests to work, I'd be in favor of splitting this PR, to just keep the changes in pybind11/critical_section.h plus the minimal tests meant to exercise that nullptrs don't cause segfaults.

XuehaiPan · 2025-06-04T06:07:25Z

If it's troublesome to get the new comprehensive tests to work

Moved the cause of the flaky test of dynamic attributes to #5715.

test: run pytest under Python devmode #5715

Note that we should eventually address #5715 before the official Python 3.14 release.

rwgk · 2025-06-04T06:41:07Z

Everything passes, awesome!

Could you please review this analysis and distill out source code comments suitable for adding to this PR?

https://chatgpt.com/share/683fea11-df78-8008-b76d-749f9e8f0df8

rwgk

Nice! Just one minor suggestion.

@henryiii to you want to include this in RC3? (I think that could be useful.)

tests/test_scoped_critical_section.cpp

Signed-off-by: Henry Schreiner <[email protected]>

Skylion007 approved these changes May 30, 2025

View reviewed changes

XuehaiPan changed the title ~~chore: handle null py::handle for py::scoped_crititcal_section~~ chore: handle null py::handle for py::scoped_critical_section May 30, 2025

XuehaiPan force-pushed the handle-null-for-scoped_critical_section branch 2 times, most recently from 8828478 to 032fef2 Compare May 31, 2025 16:50

XuehaiPan requested a review from henryiii as a code owner May 31, 2025 17:34

XuehaiPan force-pushed the handle-null-for-scoped_critical_section branch from 58763fb to 4725667 Compare May 31, 2025 17:39

This comment was marked as outdated.

Sign in to view

XuehaiPan force-pushed the handle-null-for-scoped_critical_section branch 4 times, most recently from d6084c0 to fcfcddb Compare June 3, 2025 15:21

henryiii force-pushed the handle-null-for-scoped_critical_section branch from fcfcddb to 950c15c Compare June 3, 2025 17:07

XuehaiPan changed the title ~~chore: handle null py::handle for py::scoped_critical_section~~ test: handle null py::handle and add tests for py::scoped_critical_section Jun 3, 2025

XuehaiPan added 9 commits June 3, 2025 16:19

chore: handle null for py::scoped_critical_section

3d20d9d

test: add tests for py::scoped_critical_section

7f0b6ff

test: use assert instead of REQUIRE

f7006c4

feat: enable faulthandler for pytest

543ff87

chore: use __has_include(<barrier>)

43e7294

fix: fix segmentation fault in test

3da3d28

fix: test critical_section for no-gil only

5051829

test: run new tests only

adbc2b2

test: ensure non-empty test selection

11e037b

XuehaiPan added 4 commits June 3, 2025 16:19

fix: fix test critical_section

e2e11fd

fix: change Python 3.14.0b1/2 xfail tests to non-strict

39d0aa4

test: trigger gc manually

8b48a0c

test: mark xfail to DynamicClass

009756f

henryiii force-pushed the handle-null-for-scoped_critical_section branch from cd724f0 to 009756f Compare June 3, 2025 20:20

rwgk added 2 commits June 3, 2025 21:30

Use namespace test_scoped_critical_section_ns (standard approach to…

58f155c

… guard against name clashes).

Simplify changes in pybind11/critical_section.h and add test_nullptr_…

2ce1f8a

…combinations()

rwgk reviewed Jun 4, 2025

View reviewed changes

test: disable Python devmode in pytest

ad4203f

XuehaiPan mentioned this pull request Jun 4, 2025

test: run pytest under Python devmode #5715

Open

XuehaiPan requested a review from rwgk June 4, 2025 06:49

test: add comprehensive comments for the tests

883ac69

rwgk approved these changes Jun 4, 2025

View reviewed changes

tests/test_scoped_critical_section.cpp Outdated Show resolved Hide resolved

XuehaiPan and others added 2 commits June 4, 2025 15:30

test: add a summary comment for tests

1b76b0b

refactor: simpler impl

4b24109

Signed-off-by: Henry Schreiner <[email protected]>

rwgk approved these changes Jun 4, 2025

View reviewed changes

henryiii merged commit c786d34 into pybind:master Jun 4, 2025
82 checks passed

henryiii changed the title ~~test: handle null py::handle and add tests for py::scoped_critical_section~~ fix: handle null py::handle and add tests for py::scoped_critical_section Jun 4, 2025

github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 4, 2025

fix: handle null py::handle and add tests for py::scoped_critical_section #5706

fix: handle null py::handle and add tests for py::scoped_critical_section #5706

Uh oh!

Conversation

XuehaiPan commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

Skylion007 commented May 30, 2025

Uh oh!

rwgk commented May 30, 2025

Uh oh!

henryiii commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwgk commented May 31, 2025

Uh oh!

rwgk commented May 31, 2025

Uh oh!

henryiii commented Jun 1, 2025

Uh oh!

henryiii commented Jun 3, 2025

Uh oh!

This comment was marked as outdated.

XuehaiPan commented Jun 3, 2025

Uh oh!

rwgk commented Jun 3, 2025

Uh oh!

henryiii commented Jun 4, 2025

Uh oh!

rwgk Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rwgk commented Jun 4, 2025

Uh oh!

XuehaiPan commented Jun 4, 2025

Uh oh!

rwgk commented Jun 4, 2025

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix: handle null `py::handle` and add tests for `py::scoped_critical_section` #5706

fix: handle null `py::handle` and add tests for `py::scoped_critical_section` #5706

XuehaiPan commented May 30, 2025 •

edited

Loading

henryiii commented May 30, 2025 •

edited

Loading