classification
Title: Expose public spelling of _PyGC_FINALIZED and _PyGC_SET_FINALIZED?
Type: enhancement Stage: resolved
Components: C API Versions: Python 3.9, Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: Eric Cousineau, iritkatriel, pablogsal, pitrou, vstinner
Priority: normal Keywords:

Created on 2020-04-09 15:00 by Eric Cousineau, last changed 2021-06-15 12:51 by vstinner. This issue is now closed.

Messages (13)
msg366059 - (view) Author: Eric Cousineau (Eric Cousineau) * Date: 2020-04-09 15:00
Motivated by this downstream project issue that I am working on:
https://github.com/RobotLocomotion/drake/issues/13026

In https://bugs.python.org/issue32377, I encountered PEP 442's updated resurrection behavior when moving from supporting Python 2 to Python 3.
There, Antoine Pitrou (pitrou) said that using this API (finalized + set finalized) could work, but that I could also try recreating the wrapper object. I have not yet attempted his suggestion given that (a) wrapping code is nuanced (pybind11, inheritance, etc.) and (b) this API has been working for us for the past 2 years.

Related to this, I saw some mentions of breakage of Cython due to its usage of this API:
https://bugs.python.org/issue35081#msg330045
The breakage was mitigated by keeping this internal API exposed (so kinda public, but not really?).

Is it at all possible to considering making some of this public API?
msg366066 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-09 15:20
See also bpo-40241: "[C API] Make PyGC_Head structure opaque, or even more it to the internal C API".
msg366069 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-09 15:27
> Is it at all possible to considering making some of this public API?

In bpo-35081, I wanted to move PyGC macros to the internal C API because they are private functions, but also because they expose implementation details. Example:

#define _PyGC_PREV_MASK_FINALIZED  (1)
#define _PyGCHead_FINALIZED(g) \
    (((g)->_gc_prev & _PyGC_PREV_MASK_FINALIZED) != 0)
#define _Py_AS_GC(o) ((PyGC_Head *)(o)-1)
#define _PyGC_FINALIZED(o) \
    _PyGCHead_FINALIZED(_Py_AS_GC(o))

_Py_AS_GC(o) emits machine code which hardcodes the size and alignment of the PyGC_Head structure. If PyGC_Head is changed, machine code will crash or misbehave at least. And that happened recently: bpo-27987 changed PyGC_Head between Python 3.7.4 and 3.7.5 with commit 8766cb74e186d3820db0a855ccd780d6d84461f7.

I'm not against exposing the "feature" in the public C API. I'm only against exposing macros which "leak" implementation details. What I did recently is to add regular functions in the public C API, and keep macros and static inline functions for the internal C API.

We can for example add "int PyGC_Finalized(PyObject *obj);" function which would be opaque in term of ABI.
msg366076 - (view) Author: Eric Cousineau (Eric Cousineau) * Date: 2020-04-09 16:01
C functions sound great!

I am certainly not wed to the macros (nor do I love them), as I do not have intense performance requirements where inlining (and spilling implementation guts) is necessary.
msg366077 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-04-09 16:23
There is a specific Python function in 3.9 for it:

https://docs.python.org/3.9/library/gc.html#gc.is_finalized

So it may sense to add a function to query if an object is finalized, but I am not sure it makes sense to allow to mark an object as finalized because that could mess with the GC algorithm.

Certainly exposing the macros per se may be a bad idea because there are too many implementation details there (like where the flags are stored) so at least a new function will be needed.
msg366078 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-04-09 16:25
I will make a PR for exposing function to query if an object is finalized as for sure there is value on having that in the public API.
msg366086 - (view) Author: Eric Cousineau (Eric Cousineau) * Date: 2020-04-09 20:26
> [...] but I am not sure it makes sense to allow to mark an object as finalized because that could mess with the GC algorithm.

Actually, I would like the opposite, to mark it unfinalized to resurrect the object more than once. PEP 442 from a ways back had this effect (per bpo-32377), and Antoine updated the docs after I filed the issue.

I didn't chime in early enough to snag the "more-than-once" functionality, so I guess this is what I get for dipping into private API without asking to make it public until 2 years later... d'oh!
msg366087 - (view) Author: Eric Cousineau (Eric Cousineau) * Date: 2020-04-09 20:27
Er, to clarify: "PEP 442 had that effect" => "PEP 442 had the effect of making it resurrectable only once, but not more than that."
msg366088 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-04-09 21:06
> Actually, I would like the opposite, to mark it unfinalized to resurrect the object more than once.

That is out of contract and goes against the guarantees on the GC and can (will) cause the finalizer of the object to be executed more than once. I don't think is a good idea to expose an API that allows breaking the guarantees that we give: every object is finalized once.

Also, taking into account that someone could mark externally objects as not finalized can make things more complicated for us to maintain in the collector.
msg366093 - (view) Author: Eric Cousineau (Eric Cousineau) * Date: 2020-04-10 00:16
Aye. Using a workaround for now ("leak" the object by incrementing the refcount on first resurrection):
https://github.com/RobotLocomotion/pybind11/pull/39

I may try Antoine's suggestion later on, but will definitely reformulate this to use the public API version of `gc.is_finalized()` when it comes about.

Thanks!
msg366171 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-04-11 00:25
New changeset f13072b8a89a922285737988b086beb4b06c6648 by Pablo Galindo in branch 'master':
bpo-40241: Add PyObject_GC_IsTracked and PyObject_GC_IsFinalized to the public C-API (GH-19461)
https://github.com/python/cpython/commit/f13072b8a89a922285737988b086beb4b06c6648
msg395870 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-06-15 11:38
Can this be closed now? It seems that the query API was added and the set API was rejected.
msg395877 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-06-15 12:51
Pablo: "That is out of contract and goes against the guarantees on the GC and can (will) cause the finalizer of the object to be executed more than once. I don't think is a good idea to expose an API that allows breaking the guarantees that we give: every object is finalized once."

I concur with Pablo. We must not expose an API to "unfinalize" an object.

Irit Katriel: "Can this be closed now? It seems that the query API was added and the set API was rejected."

Right. I close the issue as rejected.
History
Date User Action Args
2021-06-15 12:51:26vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg395877

stage: resolved
2021-06-15 11:38:48iritkatrielsetnosy: + iritkatriel
messages: + msg395870
2020-04-11 00:25:22vstinnersetmessages: + msg366171
2020-04-10 00:16:19Eric Cousineausetmessages: + msg366093
2020-04-09 21:06:35pablogsalsetmessages: + msg366088
2020-04-09 20:27:13Eric Cousineausetmessages: + msg366087
2020-04-09 20:26:21Eric Cousineausetmessages: + msg366086
2020-04-09 16:25:50pablogsalsetmessages: + msg366078
2020-04-09 16:23:46pablogsalsetmessages: + msg366077
2020-04-09 16:01:40Eric Cousineausetmessages: + msg366076
2020-04-09 15:27:51vstinnersetmessages: + msg366069
2020-04-09 15:20:21vstinnersetnosy: + pitrou, vstinner, pablogsal
messages: + msg366066
2020-04-09 15:00:32Eric Cousineaucreate