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.

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

gh-118789: Add PyUnstable_Object_ClearWeakRefsNoCallbacks #118807

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented May 8, 2024

This exposes _PyWeakref_ClearWeakRefsNoCallbacks as an unstable C-API function to provide a thread-safe mechanism for clearing weakrefs without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks, such as after running finalizers like we do in subtype_dealloc. Previously they could use _PyWeakref_ClearRef on each weakref, but that's not thread-safe in the free-threaded build.


📚 Documentation preview 📚: https://cpython-previews--118807.org.readthedocs.build/

This exposes `_PyWeakref_ClearWeakRefsExceptCallbacks` as an unstable
C-API function to provide a thread-safe mechanism for clearing weakrefs
without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks,
such as after running finalizers like we do in subtype_dealloc.
Previously they could use `_PyWeakref_ClearRef` on each weakref, but
that's not thread-safe in the free-threaded build.
@vstinner
Copy link
Member

vstinner commented May 9, 2024

I suggest to rename the function to: PyObject_ClearWeakRefs().

  • I don't think that it deserves to belong to the unstable API, since it's stable for many years.
  • It doesn't belong to PyWeakref API, since the argument is a generic object.
  • I don't think that there is an use case to call the callbacks. If tomorrow, someone comes with such use case, we can add a new function. I don't think that adding a parameter for that would be useful right now.

@@ -1090,6 +1090,12 @@ _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj)
UNLOCK_WEAKREFS(obj);
}

void
PyUnstable_Weakref_ClearWeakRefsExceptCallbacks(PyObject *obj)
{
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to check if _PyType_SUPPORTS_WEAKREFS(Py_TYPE(op)) and do nothing (return) if it's not the case.

@colesbury
Copy link
Contributor Author

I suggest to rename the function to: PyObject_ClearWeakRefs()

We already have PyObject_ClearWeakRefs: it clears the weakrefs and calls callbacks. It's the more commonly used API.

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

  • Clear weakrefs and call callbacks (i.e., PyObject_ClearWeakRefs)
  • Call finalizer (i.e, PyObject_CallFinalizerFromDealloc)
  • Clear weakrefs again, but don't call callbacks (i.e., PyUnstable_Weakref_ClearWeakRefsExceptCallbacks)

The third step avoids leaking memory in case the finalizer added new weakrefs. We do this internally in subtype_dealloc, but extensions can't always delegate to subtype_dealloc.

Previously, the third step was implemented as:

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

But that's not thread-safe without the GIL.

It doesn't belong to PyWeakref API, since the argument is a generic object

I'll adjust the name and add the check you suggest.

@vstinner
Copy link
Member

Why do you want to put it in the PyUnstable API?

@colesbury
Copy link
Contributor Author

Why do you want to put it in the PyUnstable API?

That's what I understood from @Yhg1s's comments on Discord (#c-api) as the appropriate name/stability for 3.13.

I'd certainly prefer something outside of the PyUnstable API as the end state for this (i.e., in 3.14).

@vstinner
Copy link
Member

I suggest to only do the revert in 3.13, and add the function only in 3.14.

@colesbury
Copy link
Contributor Author

If we only add the function in 3.14, then there will be no way for C-API extensions that need this functionality to be made thread-safe in 3.13.

@colesbury
Copy link
Contributor Author

I'll open a https://github.com/capi-workgroup/decisions issue for 3.14

@colesbury colesbury changed the title gh-118789: Add PyUnstable_Weakref_ClearWeakRefsExceptCallbacks gh-118789: Add PyUnstable_Object_ClearWeakRefsExceptCallbacks May 10, 2024
@colesbury colesbury requested a review from markshannon as a code owner June 6, 2024 16:59
@colesbury colesbury changed the title gh-118789: Add PyUnstable_Object_ClearWeakRefsExceptCallbacks gh-118789: Add PyUnstable_Object_ClearWeakRefsNoCallbacks Jun 6, 2024
@colesbury
Copy link
Contributor Author

@encukou, would you please review this?


Clears the weakrefs for *object* without calling the callbacks.

.. versionadded:: 3.13
Copy link
Member

Choose a reason for hiding this comment

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

IMO it's too late for 3.13, we're past beta1.

Suggested change
.. versionadded:: 3.13
.. versionadded:: 3.14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Yhg1s - is it okay to make this function public in 3.13? From our discussion on Discord in May, it sounded like you were okay with this before the RC, but I'd like to make sure.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, considering the circumstances, this is reasonable to add in beta 3.

@encukou
Copy link
Member

encukou commented Jun 7, 2024

Unstable API needs tests; see colesbury#2. Otherwise, this looks good to me.

@Yhg1s, do you want this in 3.13? As mentioned in the issue, projects that reimplement subtype_dealloc need this to be thread-safe in free-threading builds.
The proper solution is (IMO) to give them API so they don't need to reimplement subtype_dealloc, but that's definitely not coming in 3.13.

@colesbury colesbury requested a review from vstinner June 17, 2024 17:22
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. For the version number (.. versionadded:: in the doc), it depends if you backport the change to 3.13. If no, you should use 3.14.

@colesbury colesbury merged commit e8752d7 into python:main Jun 18, 2024
38 checks passed
@miss-islington-app
Copy link

Thanks @colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@colesbury colesbury deleted the gh-118789-PyUnstable_Weakref_ClearWeakRefsExceptCallbacks branch June 18, 2024 13:57
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 18, 2024
…thonGH-118807)

This exposes `PyUnstable_Object_ClearWeakRefsNoCallbacks` as an unstable
C-API function to provide a thread-safe mechanism for clearing weakrefs
without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks,
such as after running finalizers like we do in subtype_dealloc.
Previously they could use `_PyWeakref_ClearRef` on each weakref, but
that's not thread-safe in the free-threaded build.

(cherry picked from commit e8752d7)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Jun 18, 2024

GH-120695 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 18, 2024
@vstinner
Copy link
Member

Congrats @colesbury :-) PyTorch should be happy with this new API if I understood correctly.

colesbury added a commit that referenced this pull request Jun 18, 2024
…H-118807) (#120695)

This exposes `PyUnstable_Object_ClearWeakRefsNoCallbacks` as an unstable
C-API function to provide a thread-safe mechanism for clearing weakrefs
without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks,
such as after running finalizers like we do in subtype_dealloc.
Previously they could use `_PyWeakref_ClearRef` on each weakref, but
that's not thread-safe in the free-threaded build.

(cherry picked from commit e8752d7)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
picnixz pushed a commit to picnixz/cpython that referenced this pull request Jun 19, 2024
…thon#118807)

This exposes `PyUnstable_Object_ClearWeakRefsNoCallbacks` as an unstable
C-API function to provide a thread-safe mechanism for clearing weakrefs
without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks,
such as after running finalizers like we do in subtype_dealloc.
Previously they could use `_PyWeakref_ClearRef` on each weakref, but
that's not thread-safe in the free-threaded build.

Co-authored-by: Petr Viktorin <encukou@gmail.com>
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
…thon#118807)

This exposes `PyUnstable_Object_ClearWeakRefsNoCallbacks` as an unstable
C-API function to provide a thread-safe mechanism for clearing weakrefs
without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks,
such as after running finalizers like we do in subtype_dealloc.
Previously they could use `_PyWeakref_ClearRef` on each weakref, but
that's not thread-safe in the free-threaded build.

Co-authored-by: Petr Viktorin <encukou@gmail.com>
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
…thon#118807)

This exposes `PyUnstable_Object_ClearWeakRefsNoCallbacks` as an unstable
C-API function to provide a thread-safe mechanism for clearing weakrefs
without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks,
such as after running finalizers like we do in subtype_dealloc.
Previously they could use `_PyWeakref_ClearRef` on each weakref, but
that's not thread-safe in the free-threaded build.

Co-authored-by: Petr Viktorin <encukou@gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…thon#118807)

This exposes `PyUnstable_Object_ClearWeakRefsNoCallbacks` as an unstable
C-API function to provide a thread-safe mechanism for clearing weakrefs
without executing callbacks.

Some C-API extensions need to clear weakrefs without calling callbacks,
such as after running finalizers like we do in subtype_dealloc.
Previously they could use `_PyWeakref_ClearRef` on each weakref, but
that's not thread-safe in the free-threaded build.

Co-authored-by: Petr Viktorin <encukou@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants