From a61f37a1b7c191bfb74b1642d5b93ad6d2722c52 Mon Sep 17 00:00:00 2001 From: 1ndahous3 <9205230+1ndahous3@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:52:54 +0300 Subject: [PATCH] Own generated native closures and retain their method definitions (#8962) * Own generated native closures and retain their method definitions Assisted-by: Codex:gpt-6 * Keep native callable GC traversal off the bind path Revert method_descriptor and classmethod_descriptor to untracked payloads. Their only object field, _method_def_owner, points to an untracked HeapMethodDef. Restore the derived Traverse for builtin_function_or_method without a clear implementation. default_dealloc calls the clear slot on every deallocation, so the custom clear allocated a Vec for each bound builtin. Use the derived Traverse for builtin_method, skipping the static class. Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Jeong YunWon --- crates/vm/src/builtins/builtin_func.rs | 6 +- crates/vm/src/builtins/descriptor.rs | 10 ++- crates/vm/src/function/method.rs | 101 ++++++++++++++++++++++++- crates/vm/src/vm/context.rs | 17 ++--- 4 files changed, 113 insertions(+), 21 deletions(-) diff --git a/crates/vm/src/builtins/builtin_func.rs b/crates/vm/src/builtins/builtin_func.rs index 61e93ad03..917b7b24f 100644 --- a/crates/vm/src/builtins/builtin_func.rs +++ b/crates/vm/src/builtins/builtin_func.rs @@ -78,7 +78,7 @@ impl PyNativeFunction { self.zelf.as_deref().or(self.module_object.as_deref()) } - pub const fn as_func(&self) -> &'static dyn PyNativeFn { + pub const fn as_func(&self) -> &dyn PyNativeFn { self.value.func } } @@ -243,10 +243,12 @@ impl PyNativeFunction { name = "builtin_method", module = false, base = PyNativeFunction, - ctx = "builtin_method_type" + ctx = "builtin_method_type", + traverse )] pub struct PyNativeMethod { pub(crate) func: PyNativeFunction, + #[pytraverse(skip)] pub(crate) class: &'static Py, } diff --git a/crates/vm/src/builtins/descriptor.rs b/crates/vm/src/builtins/descriptor.rs index ae8f0f6a1..4c7c1804d 100644 --- a/crates/vm/src/builtins/descriptor.rs +++ b/crates/vm/src/builtins/descriptor.rs @@ -39,7 +39,7 @@ pub struct PyMethodDescriptor { #[pymember(name = "__objclass__", path = "typ")] #[pymember(name = "__name__", path = "name")] pub common: PyDescriptor, - pub method: &'static PyMethodDef, + pub(crate) method: &'static PyMethodDef, // vectorcall: vector_call_func, /// Prevent HeapMethodDef from being freed while this descriptor references it pub(crate) _method_def_owner: Option, @@ -126,7 +126,8 @@ impl Callable for PyMethodDescriptor { impl PyMethodDescriptor { pub fn bind(&self, obj: PyObjectRef, ctx: &Context) -> PyRef { - self.method.build_bound_function(ctx, obj) + self.method + .build_bound_function_with_owner(ctx, obj, self._method_def_owner.clone()) } } @@ -181,7 +182,7 @@ pub struct PyClassMethodDescriptor { #[pymember(name = "__objclass__", path = "typ")] #[pymember(name = "__name__", path = "name")] pub common: PyDescriptor, - pub method: &'static PyMethodDef, + pub(crate) method: &'static PyMethodDef, pub(crate) _method_def_owner: Option, } @@ -199,7 +200,8 @@ impl PyClassMethodDescriptor { } pub fn bind(&self, obj: PyObjectRef, ctx: &Context) -> PyRef { - self.method.build_bound_function(ctx, obj) + self.method + .build_bound_function_with_owner(ctx, obj, self._method_def_owner.clone()) } } diff --git a/crates/vm/src/function/method.rs b/crates/vm/src/function/method.rs index b33da2877..d586c099b 100644 --- a/crates/vm/src/function/method.rs +++ b/crates/vm/src/function/method.rs @@ -232,13 +232,22 @@ impl PyMethodDef { &'static self, ctx: &Context, obj: PyObjectRef, + ) -> PyRef { + self.build_bound_function_with_owner(ctx, obj, None) + } + + pub(crate) fn build_bound_function_with_owner( + &'static self, + ctx: &Context, + obj: PyObjectRef, + owner: Option, ) -> PyRef { let function = PyNativeFunction { zelf: Some(obj), value: self, module_object: None, module: crate::object::PyAtomicRef::new_empty(), - _method_def_owner: None, + _method_def_owner: owner, }; PyRef::new_ref( function, @@ -383,15 +392,51 @@ impl core::fmt::Debug for PyMethodDef { // This is not a part of CPython API. // But useful to support dynamically generated methods #[pyclass(name, module = false, ctx = "method_def")] -#[derive(Debug)] pub struct HeapMethodDef { method: PyMethodDef, + _function_owner: Option>, +} + +impl core::fmt::Debug for HeapMethodDef { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + self.method.fmt(f) + } } impl HeapMethodDef { #[must_use] pub const fn new(method: PyMethodDef) -> Self { - Self { method } + Self { + method, + _function_owner: None, + } + } + + pub(crate) fn with_owned_function( + name: &'static str, + function: Box, + flags: PyMethodFlags, + doc: super::ItemDoc, + ) -> Self { + // SAFETY: the box has a stable address and is retained by this payload. + // The definition is private; all functions/descriptors referring to it + // retain this HeapMethodDef and expose no static reference to callers. + let func = unsafe { &*core::ptr::from_ref(&*function) }; + Self { + method: PyMethodDef { + name, + func, + flags, + #[cfg(feature = "doc")] + doc_off: doc.offset, + #[cfg(feature = "doc")] + doc_len: doc.len, + #[cfg(feature = "doc")] + doc_body_pending: false, + doc: doc.text, + }, + _function_owner: Some(function), + } } } @@ -439,3 +484,53 @@ pub(crate) fn init(ctx: &'static Context) { let _ = ctx.intern_str(HeapMethodDef::NAME); } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{AsObject, function::FuncArgs}; + + #[test] + fn generated_functions_own_and_release_their_native_captures() { + use alloc::sync::Arc; + let captured = Arc::new(17); + crate::Interpreter::without_stdlib(Default::default()).enter(|vm| { + let capture = captured.clone(); + let definition = vm.ctx.new_method_def( + "captured", + move |_args: FuncArgs| *capture, + PyMethodFlags::empty(), + super::super::ItemDoc::NONE, + ); + let function = definition.build_function(vm, None); + let descriptor = definition.build_method(vm.ctx.types.object_type, vm); + drop(definition); + assert_eq!(Arc::strong_count(&captured), 2); + assert_eq!( + function + .as_object() + .call((), vm) + .unwrap() + .try_into_value::(vm) + .unwrap(), + 17 + ); + let method = descriptor.bind(vm.ctx.none(), &vm.ctx); + assert!(method._method_def_owner.is_some()); + drop(function); + drop(descriptor); + assert_eq!(Arc::strong_count(&captured), 2); + assert_eq!( + method + .as_object() + .call((), vm) + .unwrap() + .try_into_value::(vm) + .unwrap(), + 17 + ); + drop(method); + assert_eq!(Arc::strong_count(&captured), 1); + }); + } +} diff --git a/crates/vm/src/vm/context.rs b/crates/vm/src/vm/context.rs index 42da0551c..56fa65feb 100644 --- a/crates/vm/src/vm/context.rs +++ b/crates/vm/src/vm/context.rs @@ -659,19 +659,12 @@ impl Context { where F: IntoPyNativeFn, { - let def = PyMethodDef { + let payload = HeapMethodDef::with_owned_function( name, - func: Box::leak(Box::new(f.into_func())), - flags: flags.with_call_convention(F::ARGS), - #[cfg(feature = "doc")] - doc_off: doc.offset, - #[cfg(feature = "doc")] - doc_len: doc.len, - #[cfg(feature = "doc")] - doc_body_pending: false, - doc: doc.text, - }; - let payload = HeapMethodDef::new(def); + Box::new(f.into_func()), + flags.with_call_convention(F::ARGS), + doc, + ); PyRef::new_ref(payload, self.types.method_def.to_owned(), None) }