From 8dd58108485224bce8eeb672ad77468add7510f5 Mon Sep 17 00:00:00 2001 From: ben Date: Wed, 13 Feb 2019 20:58:33 +1300 Subject: [PATCH 1/8] Make PyObjectRef and PyObjectWeakRef it's own type. --- vm/src/builtins.rs | 16 ++------- vm/src/obj/objstr.rs | 6 ++-- vm/src/pyobject.rs | 70 +++++++++++++++++++++++++++++++--------- vm/src/stdlib/weakref.rs | 10 +++--- vm/src/sysmodule.rs | 3 +- 5 files changed, 66 insertions(+), 39 deletions(-) diff --git a/vm/src/builtins.rs b/vm/src/builtins.rs index d3aaa6ee05..f61852d129 100644 --- a/vm/src/builtins.rs +++ b/vm/src/builtins.rs @@ -16,8 +16,8 @@ use super::obj::objstr; use super::obj::objtype; use super::pyobject::{ - AttributeProtocol, IdProtocol, PyContext, PyFuncArgs, PyObject, PyObjectPayload, PyObjectRef, - PyResult, Scope, TypeProtocol, + AttributeProtocol, IdProtocol, PyContext, PyFuncArgs, PyObjectRef, + PyResult, TypeProtocol, }; use super::stdlib::io::io_open; @@ -265,17 +265,7 @@ fn make_scope(vm: &mut VirtualMachine, locals: Option<&PyObjectRef>) -> PyObject }; // TODO: handle optional globals - // Construct new scope: - let scope_inner = Scope { - locals, - parent: None, - }; - - PyObject { - payload: PyObjectPayload::Scope { scope: scope_inner }, - typ: None, - } - .into_ref() + vm.ctx.new_scope_with_locals(None, locals) } fn builtin_format(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { diff --git a/vm/src/obj/objstr.rs b/vm/src/obj/objstr.rs index a0b651f5e7..185c406060 100644 --- a/vm/src/obj/objstr.rs +++ b/vm/src/obj/objstr.rs @@ -1,6 +1,6 @@ use super::super::format::{FormatParseError, FormatPart, FormatString}; use super::super::pyobject::{ - PyContext, PyFuncArgs, PyObject, PyObjectPayload, PyObjectRef, PyResult, TypeProtocol, + PyContext, PyFuncArgs, PyObjectPayload, PyObjectRef, PyResult, TypeProtocol, }; use super::super::vm::VirtualMachine; use super::objint; @@ -1126,8 +1126,8 @@ pub fn subscript(vm: &mut VirtualMachine, value: &str, b: PyObjectRef) -> PyResu // help get optional string indices fn get_slice( - start: Option<&std::rc::Rc>>, - end: Option<&std::rc::Rc>>, + start: Option<&PyObjectRef>, + end: Option<&PyObjectRef>, len: usize, ) -> Result<(usize, usize), String> { let start_idx = match start { diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 4ec2058e4d..982a48790d 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -38,6 +38,7 @@ use std::cell::RefCell; use std::collections::HashMap; use std::fmt; use std::rc::{Rc, Weak}; +use std::ops::Deref; /* Python objects and references. @@ -54,20 +55,48 @@ Basically reference counting, but then done by rust. */ /* -The PyRef type implements +The PyObjectRef type implements https://doc.rust-lang.org/std/cell/index.html#introducing-mutability-inside-of-something-immutable */ -pub type PyRef = Rc>; /// The `PyObjectRef` is one of the most used types. It is a reference to a /// python object. A single python object can have multiple references, and /// this reference counting is accounted for by this type. Use the `.clone()` /// method to create a new reference and increment the amount of references /// to the python object by 1. -pub type PyObjectRef = PyRef; +#[derive(Debug, Clone)] +pub struct PyObjectRef { + rc: Rc> +} + +impl PyObjectRef { + pub fn downgrade(this: &Self) -> PyObjectWeakRef { + PyObjectWeakRef { weak: Rc::downgrade(&this.rc) } + } + + pub fn strong_count(this: &Self) -> usize { + Rc::strong_count(&this.rc) + } +} -/// Same as PyObjectRef, except for being a weak reference. -pub type PyObjectWeakRef = Weak>; +impl Deref for PyObjectRef { + type Target = RefCell; + + fn deref(&self) -> &Self::Target { + &self.rc + } +} + +#[derive(Debug, Clone)] +pub struct PyObjectWeakRef { + weak: Weak> +} + +impl PyObjectWeakRef { + pub fn upgrade(&self) -> Option { + self.weak.upgrade().map(|x| PyObjectRef { rc:x }) + } +} /// Use this type for function which return a python object or and exception. /// Both the python object and the python exception are `PyObjectRef` types @@ -164,11 +193,7 @@ pub struct Scope { } fn _nothing() -> PyObjectRef { - PyObject { - payload: PyObjectPayload::None, - typ: None, - } - .into_ref() + PyObject::new_no_type(PyObjectPayload::None) } pub fn create_type( @@ -535,12 +560,12 @@ impl PyContext { pub fn new_scope(&self, parent: Option) -> PyObjectRef { let locals = self.new_dict(); + self.new_scope_with_locals(parent, locals) + } + + pub fn new_scope_with_locals(&self, parent: Option, locals: PyObjectRef) -> PyObjectRef { let scope = Scope { locals, parent }; - PyObject { - payload: PyObjectPayload::Scope { scope }, - typ: None, - } - .into_ref() + PyObject::new_no_type(PyObjectPayload::Scope { scope }) } pub fn new_module(&self, name: &str, scope: PyObjectRef) -> PyObjectRef { @@ -697,6 +722,7 @@ impl PyContext { pub struct PyObject { pub payload: PyObjectPayload, pub typ: Option, + pub weak_refs: Vec // pub dict: HashMap, // __dict__ member } @@ -1015,6 +1041,7 @@ pub enum PyObjectPayload { }, WeakRef { referent: PyObjectWeakRef, + callback: Option, }, Instance { dict: RefCell, @@ -1072,13 +1099,24 @@ impl PyObject { payload, typ: Some(typ), // dict: HashMap::new(), // dict, + weak_refs: Vec::new() + } + .into_ref() + } + + fn new_no_type(payload: PyObjectPayload) -> PyObjectRef { + PyObject { + payload, + typ: None, + // dict: HashMap::new(), // dict, + weak_refs: Vec::new() } .into_ref() } // Move this object into a reference object, transferring ownership. pub fn into_ref(self) -> PyObjectRef { - Rc::new(RefCell::new(self)) + PyObjectRef { rc: Rc::new(RefCell::new(self)) } } } diff --git a/vm/src/stdlib/weakref.rs b/vm/src/stdlib/weakref.rs index 8f407675bb..4f4c85039b 100644 --- a/vm/src/stdlib/weakref.rs +++ b/vm/src/stdlib/weakref.rs @@ -11,7 +11,6 @@ use super::super::pyobject::{ TypeProtocol, }; use super::super::VirtualMachine; -use std::rc::Rc; pub fn mk_module(ctx: &PyContext) -> PyObjectRef { let py_mod = ctx.new_module("_weakref", ctx.new_scope(None)); @@ -25,10 +24,11 @@ pub fn mk_module(ctx: &PyContext) -> PyObjectRef { fn ref_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { // TODO: check first argument for subclass of `ref`. - arg_check!(vm, args, required = [(cls, None), (referent, None)]); - let referent = Rc::downgrade(referent); + arg_check!(vm, args, required = [(cls, None), (referent, None)], + optional = [(callback, None)]); + let referent = PyObjectRef::downgrade(referent); Ok(PyObject::new( - PyObjectPayload::WeakRef { referent }, + PyObjectPayload::WeakRef { referent, callback: callback.cloned() }, cls.clone(), )) } @@ -47,7 +47,7 @@ fn ref_call(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { } fn get_value(obj: &PyObjectRef) -> PyObjectWeakRef { - if let PyObjectPayload::WeakRef { referent } = &obj.borrow().payload { + if let PyObjectPayload::WeakRef { referent, .. } = &obj.borrow().payload { referent.clone() } else { panic!("Inner error getting weak ref {:?}", obj); diff --git a/vm/src/sysmodule.rs b/vm/src/sysmodule.rs index e5c701fd6f..5d2d7a171d 100644 --- a/vm/src/sysmodule.rs +++ b/vm/src/sysmodule.rs @@ -1,6 +1,5 @@ use obj::objtype; use pyobject::{PyContext, PyFuncArgs, PyObjectRef, PyResult, TypeProtocol}; -use std::rc::Rc; use std::{env, mem}; use vm::VirtualMachine; @@ -24,7 +23,7 @@ fn getframe(vm: &mut VirtualMachine, _args: PyFuncArgs) -> PyResult { fn sys_getrefcount(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { arg_check!(vm, args, required = [(object, None)]); - let size = Rc::strong_count(&object); + let size = PyObjectRef::strong_count(object); Ok(vm.ctx.new_int(size)) } From 0c1bee994627f9af3dce46f03532397a0779ccb5 Mon Sep 17 00:00:00 2001 From: ben Date: Sun, 17 Feb 2019 08:35:10 +1300 Subject: [PATCH 2/8] Move weakref type to obj folder, as it's a core type and needs to be referred to in core modules. --- vm/src/obj/mod.rs | 1 + vm/src/obj/objweakref.rs | 42 ++++++++++++++++++++++++++++++++++++++++ vm/src/pyobject.rs | 6 ++++++ vm/src/stdlib/weakref.rs | 41 ++------------------------------------- 4 files changed, 51 insertions(+), 39 deletions(-) create mode 100644 vm/src/obj/objweakref.rs diff --git a/vm/src/obj/mod.rs b/vm/src/obj/mod.rs index f5dff7563c..ebd3d7961c 100644 --- a/vm/src/obj/mod.rs +++ b/vm/src/obj/mod.rs @@ -28,4 +28,5 @@ pub mod objstr; pub mod objsuper; pub mod objtuple; pub mod objtype; +pub mod objweakref; pub mod objzip; diff --git a/vm/src/obj/objweakref.rs b/vm/src/obj/objweakref.rs new file mode 100644 index 0000000000..9a262aa5a8 --- /dev/null +++ b/vm/src/obj/objweakref.rs @@ -0,0 +1,42 @@ +use super::super::pyobject::{ + PyContext, PyFuncArgs, PyObject, PyObjectRef, PyObjectWeakRef, PyObjectPayload, PyResult, TypeProtocol, +}; +use super::super::vm::VirtualMachine; +use super::objtype; // Required for arg_check! to use isinstance + +fn ref_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { + // TODO: check first argument for subclass of `ref`. + arg_check!(vm, args, required = [(cls, Some(vm.ctx.type_type())), (referent, None)], + optional = [(callback, None)]); + let referent = PyObjectRef::downgrade(referent); + Ok(PyObject::new( + PyObjectPayload::WeakRef { referent, callback: callback.cloned() }, + cls.clone(), + )) +} + +/// Dereference the weakref, and check if we still refer something. +fn ref_call(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { + arg_check!(vm, args, required = [(zelf, Some(vm.ctx.weakref_type()))]); + let referent = get_value(zelf); + let py_obj = if let Some(obj) = referent.upgrade() { + obj + } else { + vm.get_none() + }; + Ok(py_obj) +} + +fn get_value(obj: &PyObjectRef) -> PyObjectWeakRef { + if let PyObjectPayload::WeakRef { referent, .. } = &obj.borrow().payload { + referent.clone() + } else { + panic!("Inner error getting weak ref {:?}", obj); + } +} + +pub fn init(context: &PyContext) { + let weakref_type = &context.weakref_type; + context.set_attr(weakref_type, "__new__", context.new_rustfunc(ref_new)); + context.set_attr(weakref_type, "__call__", context.new_rustfunc(ref_call)); +} diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 982a48790d..72ff532e19 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -28,6 +28,7 @@ use super::obj::objstr; use super::obj::objsuper; use super::obj::objtuple; use super::obj::objtype; +use super::obj::objweakref; use super::obj::objzip; use super::vm::VirtualMachine; use num_bigint::BigInt; @@ -177,6 +178,7 @@ pub struct PyContext { pub module_type: PyObjectRef, pub bound_method_type: PyObjectRef, pub member_descriptor_type: PyObjectRef, + pub weakref_type: PyObjectRef, pub object: PyObjectRef, pub exceptions: exceptions::ExceptionZoo, } @@ -229,6 +231,7 @@ impl PyContext { ); let property_type = create_type("property", &type_type, &object_type, &dict_type); let super_type = create_type("super", &type_type, &object_type, &dict_type); + let weakref_type = create_type("ref", &type_type, &object_type, &dict_type); let generator_type = create_type("generator", &type_type, &object_type, &dict_type); let bound_method_type = create_type("method", &type_type, &object_type, &dict_type); let member_descriptor_type = @@ -314,6 +317,7 @@ impl PyContext { module_type, bound_method_type, member_descriptor_type, + weakref_type, type_type, exceptions, }; @@ -345,6 +349,7 @@ impl PyContext { objbool::init(&context); objcode::init(&context); objframe::init(&context); + objweakref::init(&context); objnone::init(&context); exceptions::init(&context); context @@ -472,6 +477,7 @@ impl PyContext { pub fn member_descriptor_type(&self) -> PyObjectRef { self.member_descriptor_type.clone() } + pub fn weakref_type(&self) -> PyObjectRef { self.weakref_type.clone() } pub fn type_type(&self) -> PyObjectRef { self.type_type.clone() } diff --git a/vm/src/stdlib/weakref.rs b/vm/src/stdlib/weakref.rs index 4f4c85039b..44420d03f3 100644 --- a/vm/src/stdlib/weakref.rs +++ b/vm/src/stdlib/weakref.rs @@ -5,51 +5,14 @@ //! - [rust weak struct](https://doc.rust-lang.org/std/rc/struct.Weak.html) //! -use super::super::obj::objtype; use super::super::pyobject::{ - PyContext, PyFuncArgs, PyObject, PyObjectPayload, PyObjectRef, PyObjectWeakRef, PyResult, - TypeProtocol, + PyContext, PyObjectRef }; -use super::super::VirtualMachine; pub fn mk_module(ctx: &PyContext) -> PyObjectRef { let py_mod = ctx.new_module("_weakref", ctx.new_scope(None)); - let py_ref_class = ctx.new_class("ref", ctx.object()); - ctx.set_attr(&py_ref_class, "__new__", ctx.new_rustfunc(ref_new)); - ctx.set_attr(&py_ref_class, "__call__", ctx.new_rustfunc(ref_call)); - ctx.set_attr(&py_mod, "ref", py_ref_class); + ctx.set_attr(&py_mod, "ref", ctx.weakref_type()); py_mod } -fn ref_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { - // TODO: check first argument for subclass of `ref`. - arg_check!(vm, args, required = [(cls, None), (referent, None)], - optional = [(callback, None)]); - let referent = PyObjectRef::downgrade(referent); - Ok(PyObject::new( - PyObjectPayload::WeakRef { referent, callback: callback.cloned() }, - cls.clone(), - )) -} - -/// Dereference the weakref, and check if we still refer something. -fn ref_call(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { - // TODO: check first argument for subclass of `ref`. - arg_check!(vm, args, required = [(cls, None)]); - let referent = get_value(cls); - let py_obj = if let Some(obj) = referent.upgrade() { - obj - } else { - vm.get_none() - }; - Ok(py_obj) -} - -fn get_value(obj: &PyObjectRef) -> PyObjectWeakRef { - if let PyObjectPayload::WeakRef { referent, .. } = &obj.borrow().payload { - referent.clone() - } else { - panic!("Inner error getting weak ref {:?}", obj); - } -} From eab6e2f3b2bfe27c773c9c45426cb22fc3ac2d33 Mon Sep 17 00:00:00 2001 From: ben Date: Sun, 17 Feb 2019 13:23:36 +1300 Subject: [PATCH 3/8] Add callback to object being reference. --- vm/src/obj/objweakref.rs | 22 ++++++++++++++++++---- vm/src/pyobject.rs | 25 +++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/vm/src/obj/objweakref.rs b/vm/src/obj/objweakref.rs index 9a262aa5a8..2fa77d7352 100644 --- a/vm/src/obj/objweakref.rs +++ b/vm/src/obj/objweakref.rs @@ -8,11 +8,13 @@ fn ref_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { // TODO: check first argument for subclass of `ref`. arg_check!(vm, args, required = [(cls, Some(vm.ctx.type_type())), (referent, None)], optional = [(callback, None)]); - let referent = PyObjectRef::downgrade(referent); - Ok(PyObject::new( - PyObjectPayload::WeakRef { referent, callback: callback.cloned() }, + let weak_referent = PyObjectRef::downgrade(referent); + let weakref = PyObject::new( + PyObjectPayload::WeakRef { referent: weak_referent, callback: callback.cloned() }, cls.clone(), - )) + ); + referent.borrow_mut().add_weakref(&weakref); + Ok(weakref) } /// Dereference the weakref, and check if we still refer something. @@ -35,6 +37,18 @@ fn get_value(obj: &PyObjectRef) -> PyObjectWeakRef { } } +pub fn notify_weak_ref(vm: &mut VirtualMachine, obj: &PyObjectRef) -> PyResult { + if let PyObjectPayload::WeakRef { callback, .. } = &obj.borrow().payload { + if let Some(callback) = callback { + vm.invoke(callback.clone(), PyFuncArgs::empty()) + } else { + Ok(vm.get_none()) + } + } else { + panic!("Inner error getting weak ref callback {:?}", obj); + } +} + pub fn init(context: &PyContext) { let weakref_type = &context.weakref_type; context.set_attr(weakref_type, "__new__", context.new_rustfunc(ref_new)); diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 72ff532e19..a7ea947a39 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -88,6 +88,19 @@ impl Deref for PyObjectRef { } } +impl Drop for PyObjectRef { + fn drop(&mut self) { + if PyObjectRef::strong_count(self) == 1 { + // the PyObject is going to be dropped + for weak_ref in self.borrow().weak_refs.iter().rev() { +// if Err(err) = objweakref::notify_weak_ref(vm, weak_ref) { +// exceptions::print_exception(vm, err); +// } + } + } + } +} + #[derive(Debug, Clone)] pub struct PyObjectWeakRef { weak: Weak> @@ -728,7 +741,7 @@ impl PyContext { pub struct PyObject { pub payload: PyObjectPayload, pub typ: Option, - pub weak_refs: Vec + weak_refs: Vec // pub dict: HashMap, // __dict__ member } @@ -922,6 +935,10 @@ impl PyFuncArgs { PyFuncArgs { args, kwargs } } + pub fn empty() -> PyFuncArgs { + PyFuncArgs { args: vec![], kwargs: vec![] } + } + pub fn insert(&self, item: PyObjectRef) -> PyFuncArgs { let mut args = PyFuncArgs { args: self.args.clone(), @@ -1121,9 +1138,13 @@ impl PyObject { } // Move this object into a reference object, transferring ownership. - pub fn into_ref(self) -> PyObjectRef { + fn into_ref(self) -> PyObjectRef { PyObjectRef { rc: Rc::new(RefCell::new(self)) } } + + pub fn add_weakref(&mut self, weakref: &PyObjectRef) { + self.weak_refs.push(weakref.clone()) + } } #[cfg(test)] From 435510570cfd5559a8de4d35117354c0f4f87294 Mon Sep 17 00:00:00 2001 From: ben Date: Tue, 19 Feb 2019 20:20:47 +1300 Subject: [PATCH 4/8] Queue up objects to be deleted to allow for weakref callbacks --- vm/src/frame.rs | 2 ++ vm/src/obj/objweakref.rs | 8 ++++++ vm/src/pyobject.rs | 54 ++++++++++++++++++++++++++++++++++------ 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 0400bc9fba..48c8063524 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -140,6 +140,8 @@ impl Frame { } }; + PyObjectRef::process_deletes(vm); + vm.current_frame = prev_frame; value } diff --git a/vm/src/obj/objweakref.rs b/vm/src/obj/objweakref.rs index 2fa77d7352..32180549a4 100644 --- a/vm/src/obj/objweakref.rs +++ b/vm/src/obj/objweakref.rs @@ -37,6 +37,14 @@ fn get_value(obj: &PyObjectRef) -> PyObjectWeakRef { } } +pub fn clear_weak_ref(obj: &PyObjectRef) { + if let PyObjectPayload::WeakRef { ref mut referent, .. } = &mut obj.borrow_mut().payload { + referent.clear(); + } else { + panic!("Inner error getting weak ref {:?}", obj); + } +} + pub fn notify_weak_ref(vm: &mut VirtualMachine, obj: &PyObjectRef) -> PyResult { if let PyObjectPayload::WeakRef { callback, .. } = &obj.borrow().payload { if let Some(callback) = callback { diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index a7ea947a39..7137b04df4 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -36,10 +36,12 @@ use num_bigint::ToBigInt; use num_complex::Complex64; use num_traits::{One, Zero}; use std::cell::RefCell; -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::fmt; use std::rc::{Rc, Weak}; use std::ops::Deref; +use std::ptr; +use std::mem; /* Python objects and references. @@ -70,6 +72,11 @@ pub struct PyObjectRef { rc: Rc> } +// TODO support multiple virtual machines, how? +thread_local! { + static DELETE_QUEUE: RefCell> = RefCell::new(VecDeque::new()); +} + impl PyObjectRef { pub fn downgrade(this: &Self) -> PyObjectWeakRef { PyObjectWeakRef { weak: Rc::downgrade(&this.rc) } @@ -78,6 +85,35 @@ impl PyObjectRef { pub fn strong_count(this: &Self) -> usize { Rc::strong_count(&this.rc) } + + pub fn process_deletes(vm: &mut VirtualMachine) { + while let Some(obj) = DELETE_QUEUE.with(|q| q.borrow_mut().pop_front()) { + // check is still needs to be deleted + if PyObjectRef::strong_count(&obj) == 1 { + for weak_ref in obj.borrow().weak_refs.iter() { + objweakref::clear_weak_ref(weak_ref); + } + + for weak_ref in obj.borrow().weak_refs.iter().rev() { + if let Err(err) = objweakref::notify_weak_ref(vm, weak_ref) { + exceptions::print_exception(vm, &err); + } + } + + assert_eq!(PyObjectRef::strong_count(&obj), 1, + "Reference count of {:?} changed during execution of callbacks of weak references", + &obj); + + let mut manually_drop = mem::ManuallyDrop::new(obj); + + // dispose of value without calling normal drop + unsafe { + let rc_ptr = &mut manually_drop.rc as * mut _; + ptr::drop_in_place(rc_ptr); + } + } + } + } } impl Deref for PyObjectRef { @@ -90,12 +126,12 @@ impl Deref for PyObjectRef { impl Drop for PyObjectRef { fn drop(&mut self) { - if PyObjectRef::strong_count(self) == 1 { - // the PyObject is going to be dropped - for weak_ref in self.borrow().weak_refs.iter().rev() { -// if Err(err) = objweakref::notify_weak_ref(vm, weak_ref) { -// exceptions::print_exception(vm, err); -// } + if PyObjectRef::strong_count(self) == 1 && !self.borrow().weak_refs.is_empty() { + // The PyObject is going to be dropped, delete it later when we have access to vm. + // This will error when the tls is destroyed, thus meaning objects won't actually + // be properly destroyed. + if let Err(_) = DELETE_QUEUE.try_with(|q| q.borrow_mut().push_back(self.clone())) { + warn!("Object hasn't been cleaned up {:?}.", self); } } } @@ -110,6 +146,10 @@ impl PyObjectWeakRef { pub fn upgrade(&self) -> Option { self.weak.upgrade().map(|x| PyObjectRef { rc:x }) } + + pub fn clear(&mut self) { + self.weak = Weak::default(); + } } /// Use this type for function which return a python object or and exception. From 195a56a66b67f117471057589872ab407f72703f Mon Sep 17 00:00:00 2001 From: ben Date: Tue, 19 Feb 2019 21:27:18 +1300 Subject: [PATCH 5/8] Add more tests for weakref and fix objects not holding there weak references alive. Also run rustfmt. --- tests/snippets/weakrefs.py | 49 +++++++++++++++++++++++++++++++- vm/src/builtins.rs | 3 +- vm/src/frame.rs | 4 +++ vm/src/obj/objweakref.rs | 41 ++++++++++++++++++--------- vm/src/pyobject.rs | 58 +++++++++++++++++++++++--------------- vm/src/stdlib/weakref.rs | 5 +--- 6 files changed, 118 insertions(+), 42 deletions(-) diff --git a/tests/snippets/weakrefs.py b/tests/snippets/weakrefs.py index ffc61f4271..75274b874e 100644 --- a/tests/snippets/weakrefs.py +++ b/tests/snippets/weakrefs.py @@ -1,13 +1,60 @@ +import sys from _weakref import ref +data_holder = {} + + class X: - pass + def __init__(self, param=0): + self.param = param + + def __str__(self): + return f"param: {self.param}" a = X() b = ref(a) + +def callback(weak_ref): + assert weak_ref is c + assert b() is None, 'reference to same object is dead' + assert c() is None, 'reference is dead' + data_holder['first'] = True + + +c = ref(a, callback) + + +def never_callback(_weak_ref): + data_holder['never'] = True + + +# weakref should be cleaned up before object, so callback is never called +ref(a, never_callback) + assert callable(b) assert b() is a +assert 'first' not in data_holder +del a +assert b() is None +assert 'first' in data_holder +assert 'never' not in data_holder + +# TODO proper detection of RustPython if sys.implementation.name == 'RustPython': +if not hasattr(sys, 'implementation'): + # implementation detail that the object isn't dropped straight away + # this tests that when an object is resurrected it still acts as normal + delayed_drop = X(5) + delayed_drop_ref = ref(delayed_drop) + + delayed_drop = None + + assert delayed_drop_ref() is not None + value = delayed_drop_ref() + del delayed_drop # triggers process_deletes + + assert str(value) == "param: 5" + diff --git a/vm/src/builtins.rs b/vm/src/builtins.rs index f61852d129..c0d4635e75 100644 --- a/vm/src/builtins.rs +++ b/vm/src/builtins.rs @@ -16,8 +16,7 @@ use super::obj::objstr; use super::obj::objtype; use super::pyobject::{ - AttributeProtocol, IdProtocol, PyContext, PyFuncArgs, PyObjectRef, - PyResult, TypeProtocol, + AttributeProtocol, IdProtocol, PyContext, PyFuncArgs, PyObjectRef, PyResult, TypeProtocol, }; use super::stdlib::io::io_open; diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 48c8063524..d5c0cab163 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -844,6 +844,10 @@ impl Frame { // Assume here that locals is a dict let name = vm.ctx.new_str(name.to_string()); vm.call_method(&locals, "__delitem__", vec![name])?; + + // process possible delete + PyObjectRef::process_deletes(vm); + Ok(None) } diff --git a/vm/src/obj/objweakref.rs b/vm/src/obj/objweakref.rs index 32180549a4..199144ec8a 100644 --- a/vm/src/obj/objweakref.rs +++ b/vm/src/obj/objweakref.rs @@ -1,16 +1,24 @@ use super::super::pyobject::{ - PyContext, PyFuncArgs, PyObject, PyObjectRef, PyObjectWeakRef, PyObjectPayload, PyResult, TypeProtocol, + PyContext, PyFuncArgs, PyObject, PyObjectPayload, PyObjectRef, PyObjectWeakRef, PyResult, + TypeProtocol, }; use super::super::vm::VirtualMachine; use super::objtype; // Required for arg_check! to use isinstance fn ref_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { // TODO: check first argument for subclass of `ref`. - arg_check!(vm, args, required = [(cls, Some(vm.ctx.type_type())), (referent, None)], - optional = [(callback, None)]); + arg_check!( + vm, + args, + required = [(cls, Some(vm.ctx.type_type())), (referent, None)], + optional = [(callback, None)] + ); let weak_referent = PyObjectRef::downgrade(referent); let weakref = PyObject::new( - PyObjectPayload::WeakRef { referent: weak_referent, callback: callback.cloned() }, + PyObjectPayload::WeakRef { + referent: weak_referent, + callback: callback.cloned(), + }, cls.clone(), ); referent.borrow_mut().add_weakref(&weakref); @@ -37,23 +45,30 @@ fn get_value(obj: &PyObjectRef) -> PyObjectWeakRef { } } +fn get_callback(obj: &PyObjectRef) -> Option { + if let PyObjectPayload::WeakRef { callback, .. } = &obj.borrow().payload { + callback.as_ref().cloned() + } else { + panic!("Inner error getting weak ref callback {:?}", obj); + } +} + pub fn clear_weak_ref(obj: &PyObjectRef) { - if let PyObjectPayload::WeakRef { ref mut referent, .. } = &mut obj.borrow_mut().payload { + if let PyObjectPayload::WeakRef { + ref mut referent, .. + } = &mut obj.borrow_mut().payload + { referent.clear(); } else { panic!("Inner error getting weak ref {:?}", obj); } } -pub fn notify_weak_ref(vm: &mut VirtualMachine, obj: &PyObjectRef) -> PyResult { - if let PyObjectPayload::WeakRef { callback, .. } = &obj.borrow().payload { - if let Some(callback) = callback { - vm.invoke(callback.clone(), PyFuncArgs::empty()) - } else { - Ok(vm.get_none()) - } +pub fn notify_weak_ref(vm: &mut VirtualMachine, obj: PyObjectRef) -> PyResult { + if let Some(callback) = get_callback(&obj) { + vm.invoke(callback.clone(), PyFuncArgs::new(vec![obj], vec![])) } else { - panic!("Inner error getting weak ref callback {:?}", obj); + Ok(vm.get_none()) } } diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 7137b04df4..824013737f 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -38,10 +38,10 @@ use num_traits::{One, Zero}; use std::cell::RefCell; use std::collections::{HashMap, VecDeque}; use std::fmt; -use std::rc::{Rc, Weak}; +use std::mem; use std::ops::Deref; use std::ptr; -use std::mem; +use std::rc::{Rc, Weak}; /* Python objects and references. @@ -69,17 +69,19 @@ https://doc.rust-lang.org/std/cell/index.html#introducing-mutability-inside-of-s /// to the python object by 1. #[derive(Debug, Clone)] pub struct PyObjectRef { - rc: Rc> + rc: Rc>, } // TODO support multiple virtual machines, how? -thread_local! { +thread_local! { static DELETE_QUEUE: RefCell> = RefCell::new(VecDeque::new()); } impl PyObjectRef { pub fn downgrade(this: &Self) -> PyObjectWeakRef { - PyObjectWeakRef { weak: Rc::downgrade(&this.rc) } + PyObjectWeakRef { + weak: Rc::downgrade(&this.rc), + } } pub fn strong_count(this: &Self) -> usize { @@ -90,11 +92,18 @@ impl PyObjectRef { while let Some(obj) = DELETE_QUEUE.with(|q| q.borrow_mut().pop_front()) { // check is still needs to be deleted if PyObjectRef::strong_count(&obj) == 1 { - for weak_ref in obj.borrow().weak_refs.iter() { + let weak_refs: Vec = obj + .borrow() + .weak_refs + .iter() + .flat_map(|x| x.upgrade()) + .collect(); + + for weak_ref in weak_refs.iter() { objweakref::clear_weak_ref(weak_ref); } - for weak_ref in obj.borrow().weak_refs.iter().rev() { + for weak_ref in weak_refs.into_iter().rev() { if let Err(err) = objweakref::notify_weak_ref(vm, weak_ref) { exceptions::print_exception(vm, &err); } @@ -108,9 +117,11 @@ impl PyObjectRef { // dispose of value without calling normal drop unsafe { - let rc_ptr = &mut manually_drop.rc as * mut _; + let rc_ptr = &mut manually_drop.rc as *mut _; ptr::drop_in_place(rc_ptr); } + } else { + info!("Object {:?} resurrected", obj); } } } @@ -139,12 +150,12 @@ impl Drop for PyObjectRef { #[derive(Debug, Clone)] pub struct PyObjectWeakRef { - weak: Weak> + weak: Weak>, } impl PyObjectWeakRef { pub fn upgrade(&self) -> Option { - self.weak.upgrade().map(|x| PyObjectRef { rc:x }) + self.weak.upgrade().map(|x| PyObjectRef { rc: x }) } pub fn clear(&mut self) { @@ -530,7 +541,9 @@ impl PyContext { pub fn member_descriptor_type(&self) -> PyObjectRef { self.member_descriptor_type.clone() } - pub fn weakref_type(&self) -> PyObjectRef { self.weakref_type.clone() } + pub fn weakref_type(&self) -> PyObjectRef { + self.weakref_type.clone() + } pub fn type_type(&self) -> PyObjectRef { self.type_type.clone() } @@ -622,7 +635,11 @@ impl PyContext { self.new_scope_with_locals(parent, locals) } - pub fn new_scope_with_locals(&self, parent: Option, locals: PyObjectRef) -> PyObjectRef { + pub fn new_scope_with_locals( + &self, + parent: Option, + locals: PyObjectRef, + ) -> PyObjectRef { let scope = Scope { locals, parent }; PyObject::new_no_type(PyObjectPayload::Scope { scope }) } @@ -781,8 +798,7 @@ impl PyContext { pub struct PyObject { pub payload: PyObjectPayload, pub typ: Option, - weak_refs: Vec - // pub dict: HashMap, // __dict__ member + weak_refs: Vec, // pub dict: HashMap, // __dict__ member } pub trait IdProtocol { @@ -975,10 +991,6 @@ impl PyFuncArgs { PyFuncArgs { args, kwargs } } - pub fn empty() -> PyFuncArgs { - PyFuncArgs { args: vec![], kwargs: vec![] } - } - pub fn insert(&self, item: PyObjectRef) -> PyFuncArgs { let mut args = PyFuncArgs { args: self.args.clone(), @@ -1162,7 +1174,7 @@ impl PyObject { payload, typ: Some(typ), // dict: HashMap::new(), // dict, - weak_refs: Vec::new() + weak_refs: Vec::new(), } .into_ref() } @@ -1172,18 +1184,20 @@ impl PyObject { payload, typ: None, // dict: HashMap::new(), // dict, - weak_refs: Vec::new() + weak_refs: Vec::new(), } .into_ref() } // Move this object into a reference object, transferring ownership. fn into_ref(self) -> PyObjectRef { - PyObjectRef { rc: Rc::new(RefCell::new(self)) } + PyObjectRef { + rc: Rc::new(RefCell::new(self)), + } } pub fn add_weakref(&mut self, weakref: &PyObjectRef) { - self.weak_refs.push(weakref.clone()) + self.weak_refs.push(PyObjectRef::downgrade(weakref)) } } diff --git a/vm/src/stdlib/weakref.rs b/vm/src/stdlib/weakref.rs index 44420d03f3..0c54201953 100644 --- a/vm/src/stdlib/weakref.rs +++ b/vm/src/stdlib/weakref.rs @@ -5,9 +5,7 @@ //! - [rust weak struct](https://doc.rust-lang.org/std/rc/struct.Weak.html) //! -use super::super::pyobject::{ - PyContext, PyObjectRef -}; +use super::super::pyobject::{PyContext, PyObjectRef}; pub fn mk_module(ctx: &PyContext) -> PyObjectRef { let py_mod = ctx.new_module("_weakref", ctx.new_scope(None)); @@ -15,4 +13,3 @@ pub fn mk_module(ctx: &PyContext) -> PyObjectRef { ctx.set_attr(&py_mod, "ref", ctx.weakref_type()); py_mod } - From e89f8fafe55082143b231b79098d0643e2b1c82c Mon Sep 17 00:00:00 2001 From: ben Date: Tue, 19 Feb 2019 21:34:00 +1300 Subject: [PATCH 6/8] Fix clippy warning --- vm/src/pyobject.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 824013737f..4ff1526cb2 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -141,7 +141,7 @@ impl Drop for PyObjectRef { // The PyObject is going to be dropped, delete it later when we have access to vm. // This will error when the tls is destroyed, thus meaning objects won't actually // be properly destroyed. - if let Err(_) = DELETE_QUEUE.try_with(|q| q.borrow_mut().push_back(self.clone())) { + if DELETE_QUEUE.try_with(|q| q.borrow_mut().push_back(self.clone())).is_err() { warn!("Object hasn't been cleaned up {:?}.", self); } } From b6d28cf425de8dd68a6bb12f7dfdea3ee300dc17 Mon Sep 17 00:00:00 2001 From: ben Date: Tue, 19 Feb 2019 21:40:51 +1300 Subject: [PATCH 7/8] Formatting --- vm/src/pyobject.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 4ff1526cb2..7c2bb77b49 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -141,7 +141,10 @@ impl Drop for PyObjectRef { // The PyObject is going to be dropped, delete it later when we have access to vm. // This will error when the tls is destroyed, thus meaning objects won't actually // be properly destroyed. - if DELETE_QUEUE.try_with(|q| q.borrow_mut().push_back(self.clone())).is_err() { + if DELETE_QUEUE + .try_with(|q| q.borrow_mut().push_back(self.clone())) + .is_err() + { warn!("Object hasn't been cleaned up {:?}.", self); } } @@ -798,7 +801,8 @@ impl PyContext { pub struct PyObject { pub payload: PyObjectPayload, pub typ: Option, - weak_refs: Vec, // pub dict: HashMap, // __dict__ member + weak_refs: Vec, + // pub dict: HashMap, // __dict__ member } pub trait IdProtocol { From 350bc30311ef76fcbcfaf34a3521edd5c71eaa34 Mon Sep 17 00:00:00 2001 From: ben Date: Wed, 20 Feb 2019 20:55:25 +1300 Subject: [PATCH 8/8] Move weak_refs attribute into Instance payload --- tests/snippets/weakrefs.py | 2 ++ vm/src/obj/objtype.rs | 2 +- vm/src/obj/objweakref.rs | 11 +++++-- vm/src/pyobject.rs | 62 ++++++++++++++++++++++++-------------- 4 files changed, 51 insertions(+), 26 deletions(-) diff --git a/tests/snippets/weakrefs.py b/tests/snippets/weakrefs.py index 75274b874e..ee201bb71d 100644 --- a/tests/snippets/weakrefs.py +++ b/tests/snippets/weakrefs.py @@ -1,6 +1,7 @@ import sys from _weakref import ref +from testutils import assert_raises data_holder = {} @@ -58,3 +59,4 @@ def never_callback(_weak_ref): assert str(value) == "param: 5" +assert_raises(TypeError, lambda: ref(1), "can't create weak reference to an int") diff --git a/vm/src/obj/objtype.rs b/vm/src/obj/objtype.rs index 95c493a450..0b3be70a61 100644 --- a/vm/src/obj/objtype.rs +++ b/vm/src/obj/objtype.rs @@ -232,7 +232,7 @@ pub fn get_attributes(obj: &PyObjectRef) -> PyAttributes { } // Get instance attributes: - if let PyObjectPayload::Instance { dict } = &obj.borrow().payload { + if let PyObjectPayload::Instance { dict, .. } = &obj.borrow().payload { for (name, value) in dict.borrow().iter() { attributes.insert(name.to_string(), value.clone()); } diff --git a/vm/src/obj/objweakref.rs b/vm/src/obj/objweakref.rs index 199144ec8a..7c3eba943e 100644 --- a/vm/src/obj/objweakref.rs +++ b/vm/src/obj/objweakref.rs @@ -21,8 +21,15 @@ fn ref_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult { }, cls.clone(), ); - referent.borrow_mut().add_weakref(&weakref); - Ok(weakref) + if referent.borrow_mut().add_weakref(&weakref) { + Ok(weakref) + } else { + let referent_repr = vm.to_pystr(&referent.typ())?; + Err(vm.new_type_error(format!( + "cannot create weak reference to '{}' object", + referent_repr + ))) + } } /// Dereference the weakref, and check if we still refer something. diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 7c2bb77b49..eeba263510 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -92,12 +92,13 @@ impl PyObjectRef { while let Some(obj) = DELETE_QUEUE.with(|q| q.borrow_mut().pop_front()) { // check is still needs to be deleted if PyObjectRef::strong_count(&obj) == 1 { - let weak_refs: Vec = obj - .borrow() - .weak_refs - .iter() - .flat_map(|x| x.upgrade()) - .collect(); + let weak_refs = match obj.borrow().payload { + PyObjectPayload::Instance { ref weak_refs, .. } => weak_refs + .iter() + .flat_map(|x| x.upgrade()) + .collect::>(), + _ => panic!("Non-instance object can't have weakrefs"), + }; for weak_ref in weak_refs.iter() { objweakref::clear_weak_ref(weak_ref); @@ -137,15 +138,22 @@ impl Deref for PyObjectRef { impl Drop for PyObjectRef { fn drop(&mut self) { - if PyObjectRef::strong_count(self) == 1 && !self.borrow().weak_refs.is_empty() { - // The PyObject is going to be dropped, delete it later when we have access to vm. - // This will error when the tls is destroyed, thus meaning objects won't actually - // be properly destroyed. - if DELETE_QUEUE - .try_with(|q| q.borrow_mut().push_back(self.clone())) - .is_err() - { - warn!("Object hasn't been cleaned up {:?}.", self); + if PyObjectRef::strong_count(self) == 1 { + let has_weakrefs = match self.borrow().payload { + PyObjectPayload::Instance { ref weak_refs, .. } => !weak_refs.is_empty(), + _ => false, + }; + + if has_weakrefs { + // The PyObject is going to be dropped, delete it later when we have access to vm. + // This will error when the tls is destroyed, thus meaning objects won't actually + // be properly destroyed. + if DELETE_QUEUE + .try_with(|q| q.borrow_mut().push_back(self.clone())) + .is_err() + { + warn!("Object hasn't been cleaned up {:?}.", self); + } } } } @@ -738,6 +746,7 @@ impl PyContext { PyObject::new( PyObjectPayload::Instance { dict: RefCell::new(dict), + weak_refs: Vec::new(), }, class, ) @@ -764,7 +773,8 @@ impl PyContext { pub fn set_attr(&self, obj: &PyObjectRef, attr_name: &str, value: PyObjectRef) { match obj.borrow().payload { PyObjectPayload::Module { ref dict, .. } => self.set_attr(dict, attr_name, value), - PyObjectPayload::Instance { ref dict } | PyObjectPayload::Class { ref dict, .. } => { + PyObjectPayload::Instance { ref dict, .. } + | PyObjectPayload::Class { ref dict, .. } => { dict.borrow_mut().insert(attr_name.to_string(), value); } PyObjectPayload::Scope { ref scope } => { @@ -801,7 +811,6 @@ impl PyContext { pub struct PyObject { pub payload: PyObjectPayload, pub typ: Option, - weak_refs: Vec, // pub dict: HashMap, // __dict__ member } @@ -904,7 +913,7 @@ impl AttributeProtocol for PyObjectRef { } None } - PyObjectPayload::Instance { ref dict } => dict.borrow().get(attr_name).cloned(), + PyObjectPayload::Instance { ref dict, .. } => dict.borrow().get(attr_name).cloned(), _ => None, } } @@ -916,7 +925,7 @@ impl AttributeProtocol for PyObjectRef { PyObjectPayload::Class { ref mro, .. } => { class_has_item(self, attr_name) || mro.iter().any(|d| class_has_item(d, attr_name)) } - PyObjectPayload::Instance { ref dict } => dict.borrow().contains_key(attr_name), + PyObjectPayload::Instance { ref dict, .. } => dict.borrow().contains_key(attr_name), _ => false, } } @@ -1124,6 +1133,7 @@ pub enum PyObjectPayload { }, Instance { dict: RefCell, + weak_refs: Vec, }, RustFunction { function: Box PyResult>, @@ -1178,7 +1188,6 @@ impl PyObject { payload, typ: Some(typ), // dict: HashMap::new(), // dict, - weak_refs: Vec::new(), } .into_ref() } @@ -1188,7 +1197,6 @@ impl PyObject { payload, typ: None, // dict: HashMap::new(), // dict, - weak_refs: Vec::new(), } .into_ref() } @@ -1200,8 +1208,16 @@ impl PyObject { } } - pub fn add_weakref(&mut self, weakref: &PyObjectRef) { - self.weak_refs.push(PyObjectRef::downgrade(weakref)) + pub fn add_weakref(&mut self, weakref: &PyObjectRef) -> bool { + match self.payload { + PyObjectPayload::Instance { + ref mut weak_refs, .. + } => { + weak_refs.push(PyObjectRef::downgrade(weakref)); + true + } + _ => false, + } } }