capi-workgroup / decisions

Discussion and voting on specific issues
5 stars 1 forks source link

Add `PyUnstable_Object_ClearWeakRefsNoCallbacks()` function #25

Closed colesbury closed 3 months ago

colesbury commented 4 months ago

Background

The use case that types with user defined finalizers (i.e., __del__ methods) need to perform the following sequence if they want to be correct:

The third-step is performed internally via _PyWeakref_ClearWeakRefsExceptCallbacks, but that's not a public API. This is performed internally by subtype_dealloc, but extensions cannot always delegate to that function.

Previously, C API extensions used the following pattern, but it's not thread-safe without the GIL and relies on an undocumented private API _PyWeakref_ClearRef:

      PyWeakReference **list = PyObject_GET_WEAKREFS_LISTPTR(self);
      while (*list) {
        _PyWeakref_ClearRef(*list);
      }

Proposed API

// Clears the weakrefs for object `obj`, but does not call their callbacks.
PyAPI_FUNC(void) PyObject_ClearWeakRefsNoCallbacks(PyObject *obj);

Implementation

The implementation already exists as the internal-only _PyWeakref_ClearWeakRefsExceptCallbacks function. Per @vstinner's suggestion, I'd propose that the PyObject_ClearWeakRefsNoCallbacks returns immediately if obj does not support weakrefs.

See also

EDIT: Updated name based on @iritkatriel's suggestion

gvanrossum commented 4 months ago

No objection from me.

iritkatriel commented 4 months ago

I don't think the name PyObject_ClearWeakRefsExceptCallbacks is clear. Except in particular.

Something like PyObject_ClearWeakRefsNoCallbacks maybe.

colesbury commented 4 months ago

I've updated the proposed name

vstinner commented 4 months ago

LGTM.

gvanrossum commented 4 months ago

WFM.

--Guido (mobile)

On Sun, May 12, 2024 at 00:23 Victor Stinner @.***> wrote:

LGTM.

— Reply to this email directly, view it on GitHub https://github.com/capi-workgroup/decisions/issues/25#issuecomment-2106149006 or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWCWMX7KND3JZV5IRQBBBTZB4KFVBFKMF2HI4TJMJ2XIZLTSOBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLLDTOVRGUZLDORPXI6LQMWWES43TOVSUG33NNVSW45FGORXXA2LDOOJIFJDUPFYGLKTSMVYG643JORXXE6NFOZQWY5LFVE3TENZWHEZDENRQQKSHI6LQMWSWS43TOVS2K5TBNR2WLKRSGI4TAMJVGI4DKM5HORZGSZ3HMVZKMY3SMVQXIZI . You are receiving this email because you commented on the thread.

Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub .

encukou commented 4 months ago

While the semantics of PyObject_ClearWeakRefsNoCallbacks function itself are clear, it seems that anyone calling it is quite deep in implementation details. I worry that by exposing the function, we're implicitly saying that it's OK to re-implementing subtype_dealloc, or at least the “clear/call del/clear again” dance.

It seems better to expose this as PyUnstable_Object_ClearWeakRefsNoCallbacks: if you use this function, you better be checking What's News and you better be prepared to adjust code for each new release. (Perhaps one of those releases will add a PyObject_Finalize() that abstracts away a well-defined chunk of subtype_dealloc. But even if we add something like that in 3.14, a transitional PyUnstable_Object_ClearWeakRefsNoCallbacks would be nice to have.)

colesbury commented 4 months ago

PyUnstable_Object_ClearWeakRefsNoCallbacks is fine with me too.

colesbury commented 4 months ago

@vstinner - is PyUnstable_Object_ClearWeakRefsNoCallbacks okay with you?

vstinner commented 4 months ago

I worry that by exposing the function, we're implicitly saying that it's OK to re-implementing subtype_dealloc, or at least the “clear/call del/clear again” dance.

IMO it's ok to reimplement something like subtype_dealloc, since 3rd party extensions do it, and that's why Sam wants to make the function public. I'm not sure that the function needs to be tagged as "unstable". The documentation can be clear about where and when it should be used or not.

vstinner commented 4 months ago

It seems like we are ok with the feature, but not in which API it should be added: how the function should be named. I'm not sure how to resolve this disagreement, so I propose a poll/vote below.

Function name: please pick only name :-)

PyUnstable_Object_ClearWeakRefsNoCallbacks() - unstable API, rationale: https://github.com/capi-workgroup/decisions/issues/25#issuecomment-2107728647

PyObject_ClearWeakRefsNoCallbacks() - public API, rationale: https://github.com/capi-workgroup/decisions/issues/25#issuecomment-2125451169

encukou commented 4 months ago

IMO it's ok to reimplement something like subtype_dealloc, since 3rd party extensions do it[...] The documentation can be clear about where and when it should be used or not.

I'd need to see that documentation to be convinced.

zooba commented 4 months ago

I lean towards the "unstable" marking, just so we can remove it within a release. I wouldn't expect the API to change in any other meaningful way apart from the function no longer doing what the caller thinks it is doing.

erlend-aasland commented 4 months ago

I chose PyObject_ClearWeakRefsNoCallbacks, but I won't oppose the unstable variant (though I think it is a weak candidate for the unstable tier).

encukou commented 3 months ago

We discussed this on a meeting, and decided that it should be unstable API. Please go with PyUnstable_Object_ClearWeakRefsNoCallbacks.