gh-128844: Make `_Py_TryIncref` public as an unstable API. by colesbury · Pull Request #128926 · python/cpython

colesbury · 2025-01-16T21:58:46Z

This exposes _Py_TryIncref as PyUnstable_TryIncref() and the helper function _PyObject_SetMaybeWeakref as PyUnstable_EnableTryIncRef.

These are helpers for dealing with unowned references in a safe way, particularly in the free threading build.

Issue: Make _Py_TryIncref public as an unstable API as PyUnstable_TryIncref() #128844

📚 Documentation preview 📚: https://cpython-previews--128926.org.readthedocs.build/en/128926/c-api/object.html#c.PyUnstable_TryIncRef

This exposes `_Py_TryIncref` as `PyUnstable_TryIncref()` and the helper function `_PyObject_SetMaybeWeakref` as `PyUnstable_EnableTryIncRef`. These are helpers for dealing with unowned references in a safe way, particularly in the free threading build.

Objects/object.c

corona10 · 2025-01-17T13:58:40Z

Doc/c-api/object.rst

+
+   .. versionadded:: 3.14
+
+.. c:function:: int PyUnstable_EnableTryIncRef(PyObject *obj)


Maybe void?

vstinner

LGTM. I just have a suggestion for the test.

Modules/_testcapi/object.c

kumaraditya303 · 2025-01-17T18:13:44Z

Modules/_testcapi/object.c

+
+    PyUnstable_EnableTryIncRef(op);
+#ifdef Py_GIL_DISABLED
+    // PyUnstable_EnableTryIncRef sets the shared flags to *at least*


This comment seems incomplete, can you elaborate it a bit more?

kumaraditya303 · 2025-01-17T18:17:00Z

Doc/c-api/object.rst

+   This is intended as a building block for safely dealing with unowned
+   references without the overhead of creating a :c:type:`!PyWeakReference`.


In C API docs the term borrowed reference is more common than unowned reference, can you use that?

There's a glossary entry for it so it can be linked to that.

I'll update the term, but I don't love the use of "borrowed reference" here. This seems different to me than the typical use of "borrowed reference" -- there's not a particular reference that's borrowed here. It's more like a building block for weak reference like objects without using PyWeakReference.

cpython/Include/cpython/weakrefobject.h

Line 12 in d95ba9f

* Note that this is a stealth reference: wr_object's refcount is

encukou · 2025-01-18T06:58:19Z

Please don't merge yet, I'd like to try tweaking the docs wording, early next week

…

On January 18, 2025 6:57:08 AM GMT+01:00, Donghee Na ***@***.***> wrote: @corona10 approved this pull request. -- Reply to this email directly or view it on GitHub: #128926 (review) You are receiving this because your review was requested. Message ID: ***@***.***>

encukou · 2025-01-20T13:39:51Z

Doc/c-api/object.rst

+   This is intended as a building block for safely dealing with
+   :term:`borrowed references <borrowed reference>` without the overhead of
+   creating a :c:type:`!PyWeakReference`.


I think this should use weak reference, as a generic term. The “without the overhead” note implies that we don't mean the concrete PyWeakReference.
“Borrowed reference” has a specific meaning in CPython (in particular, obj of PyUnstable_TryIncRef is a borrowed reference).
“Unowned reference” would, IMO, need a definition.

I'd also warn that you typically need to control the deallocator (both TryIncRef and tp_dealloc need the same lock). Is that correct?

Suggested change

This is intended as a building block for safely dealing with

:term:`borrowed references <borrowed reference>` without the overhead of

creating a :c:type:`!PyWeakReference`.

This is intended as a building block for managing weak references without the overhead of

creating a :c:type:`!PyWeakReference`.

Typically, correct use of this function requires support from *obj*'s

deallocator (:c:member:`~PyTypeObject.tp_dealloc`).

Here is a larger change, with an example adapted from the issue (itself based on pybind11). I personally didn't “get” the function before seeing the example, but I don't know if I'm the target audience. I'm also not sure if there are other use cases this doesn't cover.

Thanks, I think this is better, especially with the example. I think we should also include the use of PyUnstable_EnableTryIncref in the example.

I've updated the branch.

colesbury · 2025-01-27T19:46:40Z

@encukou - would you please take another look at this when you get a chance?

encukou

Looks good, thank you!
One more nitpick.

Doc/c-api/object.rst

Co-authored-by: Petr Viktorin <encukou@gmail.com>

vstinner · 2025-01-28T22:32:06Z

Congrats Sam! Honestly, this API is weird/surprising (trying to INCREF, hu?), but it looks useful for Free Threading. The doc is nice, thanks @encukou for the example.

colesbury added topic-C-API topic-free-threading labels Jan 16, 2025

bedevere-app bot mentioned this pull request Jan 16, 2025

Make _Py_TryIncref public as an unstable API as PyUnstable_TryIncref() #128844

Closed

colesbury added 3 commits January 16, 2025 22:22

Fix doc

12ea212

Ignore global variables in test code

2e18b45

Remove incomplete sentence from docs

cba2d54

corona10 requested review from encukou and vstinner January 17, 2025 13:55

corona10 reviewed Jan 17, 2025

View reviewed changes

Objects/object.c Show resolved Hide resolved

corona10 reviewed Jan 17, 2025

View reviewed changes

vstinner approved these changes Jan 17, 2025

View reviewed changes

Modules/_testcapi/object.c Show resolved Hide resolved

bedevere-app bot added the awaiting merge label Jan 17, 2025

colesbury marked this pull request as ready for review January 17, 2025 18:00

colesbury requested a review from ericsnowcurrently as a code owner January 17, 2025 18:00

bedevere-app bot added awaiting core review and removed awaiting merge labels Jan 17, 2025

colesbury added 2 commits January 17, 2025 13:01

Fix return type in docs

31d0548

Reset MyObject_dealloc_called at start of test_py_try_inc_ref

9ec47b3

kumaraditya303 reviewed Jan 17, 2025

View reviewed changes

colesbury added 2 commits January 17, 2025 19:59

Fixup comment

8e53a2e

s/unowned/borrowed

37a08dc

corona10 approved these changes Jan 18, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Jan 18, 2025

kumaraditya303 approved these changes Jan 18, 2025

View reviewed changes

encukou reviewed Jan 20, 2025

View reviewed changes

Add an example sketch from Petr Viktorin

405952c

colesbury force-pushed the gh-1288440-try-incref branch from f5507d8 to 405952c Compare January 20, 2025 15:08

encukou and others added 2 commits January 21, 2025 12:01

Add PyUnstable_EnableTryIncRef to the example

cac2eb4

Add locking to add_entry

887ab77

colesbury requested a review from encukou January 22, 2025 14:51

Merge branch 'main' into gh-1288440-try-incref

95cad2d

encukou approved these changes Jan 28, 2025

View reviewed changes

Doc/c-api/object.rst Outdated Show resolved Hide resolved

Update Doc/c-api/object.rst

4a87acb

Co-authored-by: Petr Viktorin <encukou@gmail.com>

colesbury force-pushed the gh-1288440-try-incref branch from 78d29a3 to 4a87acb Compare January 28, 2025 14:48

Merge branch 'main' into gh-1288440-try-incref

4161242

colesbury enabled auto-merge (squash) January 28, 2025 19:09

colesbury merged commit d23f570 into python:main Jan 28, 2025
41 checks passed

bedevere-app bot removed the awaiting merge label Jan 28, 2025


		.. versionadded:: 3.14

		.. c:function:: int PyUnstable_EnableTryIncRef(PyObject *obj)

		This is intended as a building block for safely dealing with unowned
		references without the overhead of creating a :c:type:`!PyWeakReference`.

Uh oh!

Conversation

colesbury commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

corona10 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kumaraditya303 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

kumaraditya303 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

colesbury Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

encukou commented Jan 18, 2025 via email

Uh oh!

encukou Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

colesbury Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

encukou Jan 21, 2025

Choose a reason for hiding this comment

Uh oh!

colesbury commented Jan 27, 2025

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

colesbury commented Jan 16, 2025 •

edited

Loading