remove risky finalization code from Dispatcher, and use reference types

lostmsu · lostmsu · commit 391793411133 · 2021-09-21T16:08:27.000-07:00
diff --git a/src/runtime/converter.cs b/src/runtime/converter.cs
@@ -112,6 +112,8 @@ internal static IntPtr ToPython<T>(T value)
 
         internal static NewReference ToPythonReference<T>(T value)
             => NewReference.DangerousFromPointer(ToPython(value, typeof(T)));
+        internal static NewReference ToPythonReference(object value, Type type)
+            => NewReference.DangerousFromPointer(ToPython(value, type));
 
         private static readonly Func<object, bool> IsTransparentProxy = GetIsTransparentProxy();
 
diff --git a/src/runtime/delegatemanager.cs b/src/runtime/delegatemanager.cs
@@ -29,21 +29,6 @@ public DelegateManager()
             dispatch = basetype.GetMethod("Dispatch");
         }
 
-        /// <summary>
-        /// Given a true delegate instance, return the PyObject handle of the
-        /// Python object implementing the delegate (or IntPtr.Zero if the
-        /// delegate is not implemented in Python code.
-        /// </summary>
-        public IntPtr GetPythonHandle(Delegate d)
-        {
-            if (d?.Target is Dispatcher)
-            {
-                var disp = (Dispatcher)d.Target;
-                return disp.target;
-            }
-            return IntPtr.Zero;
-        }
-
         /// <summary>
         /// GetDispatcher is responsible for creating a class that provides
         /// an appropriate managed callback method for a given delegate type.
@@ -224,41 +209,15 @@ A possible alternate strategy would be to create custom subclasses
 
     public class Dispatcher
     {
-        public IntPtr target;
-        public Type dtype;
-        private bool _disposed = false;
-        private bool _finalized = false;
+        readonly PyObject target;
+        readonly Type dtype;
 
         public Dispatcher(IntPtr target, Type dtype)
         {
-            Runtime.XIncref(target);
-            this.target = target;
+            this.target = new PyObject(new BorrowedReference(target));
             this.dtype = dtype;
         }
 
-        ~Dispatcher()
-        {
-            if (_finalized || _disposed)
-            {
-                return;
-            }
-            _finalized = true;
-            Finalizer.Instance.AddFinalizedObject(ref target);
-        }
-
-        public void Dispose()
-        {
-            if (_disposed)
-            {
-                return;
-            }
-            _disposed = true;
-            Runtime.XDecref(target);
-            target = IntPtr.Zero;
-            dtype = null;
-            GC.SuppressFinalize(this);
-        }
-
         public object Dispatch(object[] args)
         {
             IntPtr gs = PythonEngine.AcquireLock();
@@ -280,26 +239,36 @@ private object TrueDispatch(object[] args)
         {
             MethodInfo method = dtype.GetMethod("Invoke");
             ParameterInfo[] pi = method.GetParameters();
-            IntPtr pyargs = Runtime.PyTuple_New(pi.Length);
             Type rtype = method.ReturnType;
 
-            for (var i = 0; i < pi.Length; i++)
+            NewReference op;
+            using (var pyargs = NewReference.DangerousFromPointer(Runtime.PyTuple_New(pi.Length)))
             {
-                // Here we own the reference to the Python value, and we
-                // give the ownership to the arg tuple.
-                IntPtr arg = Converter.ToPython(args[i], pi[i].ParameterType);
-                Runtime.PyTuple_SetItem(pyargs, i, arg);
-            }
+                for (var i = 0; i < pi.Length; i++)
+                {
+                    // Here we own the reference to the Python value, and we
+                    // give the ownership to the arg tuple.
+                    var arg = Converter.ToPythonReference(args[i], pi[i].ParameterType);
+                    if (arg.IsNull())
+                    {
+                        throw PythonException.ThrowLastAsClrException();
+                    }
+                    int res = Runtime.PyTuple_SetItem(pyargs, i, arg.Steal());
+                    if (res != 0)
+                    {
+                        throw PythonException.ThrowLastAsClrException();
+                    }
+                }
 
-            IntPtr op = Runtime.PyObject_Call(target, pyargs, IntPtr.Zero);
-            Runtime.XDecref(pyargs);
+                op = Runtime.PyObject_Call(target.Reference, pyargs, BorrowedReference.Null);
+            }
 
-            if (op == IntPtr.Zero)
+            if (op.IsNull())
             {
                 throw PythonException.ThrowLastAsClrException();
             }
 
-            try
+            using (op)
             {
                 int byRefCount = pi.Count(parameterInfo => parameterInfo.ParameterType.IsByRef);
                 if (byRefCount > 0)
@@ -339,7 +308,7 @@ private object TrueDispatch(object[] args)
                             Type t = pi[i].ParameterType;
                             if (t.IsByRef)
                             {
-                                IntPtr item = Runtime.PyTuple_GetItem(op, index++);
+                                BorrowedReference item = Runtime.PyTuple_GetItem(op, index++);
                                 if (!Converter.ToManaged(item, t, out object newArg, true))
                                 {
                                     Exceptions.RaiseTypeError($"The Python function returned a tuple where element {i} was not {t.GetElementType()} (the out parameter type)");
@@ -352,7 +321,7 @@ private object TrueDispatch(object[] args)
                         {
                             return null;
                         }
-                        IntPtr item0 = Runtime.PyTuple_GetItem(op, 0);
+                        BorrowedReference item0 = Runtime.PyTuple_GetItem(op, 0);
                         if (!Converter.ToManaged(item0, rtype, out object result0, true))
                         {
                             Exceptions.RaiseTypeError($"The Python function returned a tuple where element 0 was not {rtype} (the return type)");
@@ -397,10 +366,6 @@ private object TrueDispatch(object[] args)
 
                 return result;
             }
-            finally
-            {
-                Runtime.XDecref(op);
-            }
         }
     }
 }
diff --git a/src/runtime/importhook.cs b/src/runtime/importhook.cs
@@ -120,7 +120,7 @@ static void SetupImportHook()
             var mod_dict = Runtime.PyModule_GetDict(import_hook_module);
             // reference not stolen due to overload incref'ing for us.
             Runtime.PyTuple_SetItem(args, 1, mod_dict);
-            Runtime.PyObject_Call(exec, args, default);
+            Runtime.PyObject_Call(exec, args, default).Dispose();
             // Set as a sub-module of clr.
             if(Runtime.PyModule_AddObject(ClrModuleReference, "loader", import_hook_module.DangerousGetAddress()) != 0)
             {
diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs
@@ -1003,6 +1003,8 @@ internal static IntPtr PyObject_Type(IntPtr op)
         internal static NewReference PyObject_Type(BorrowedReference o)
             => Delegates.PyObject_Type(o);
 
+        internal static string PyObject_GetTypeName(BorrowedReference op)
+            => PyObject_GetTypeName(op.DangerousGetAddress());
         internal static string PyObject_GetTypeName(IntPtr op)
         {
             IntPtr pyType = PyObject_TYPE(op);
@@ -1087,8 +1089,8 @@ internal static IntPtr PyObject_GetAttr(IntPtr pointer, IntPtr name)
 
 
         internal static IntPtr PyObject_Call(IntPtr pointer, IntPtr args, IntPtr kw) => Delegates.PyObject_Call(pointer, args, kw);
-        internal static IntPtr PyObject_Call(BorrowedReference pointer, BorrowedReference args, BorrowedReference kw) 
-            => Delegates.PyObject_Call(pointer.DangerousGetAddress(), args.DangerousGetAddress(), kw.DangerousGetAddressOrNull());
+        internal static NewReference PyObject_Call(BorrowedReference pointer, BorrowedReference args, BorrowedReference kw) 
+            => NewReference.DangerousFromPointer(Delegates.PyObject_Call(pointer.DangerousGetAddress(), args.DangerousGetAddress(), kw.DangerousGetAddressOrNull()));
 
 
         internal static NewReference PyObject_CallObject(BorrowedReference callable, BorrowedReference args) => Delegates.PyObject_CallObject(callable, args);

Original file line number	Diff line number	Diff line change
`@@ -120,7 +120,7 @@ static void SetupImportHook()`
`120`	`120`	`var mod_dict = Runtime.PyModule_GetDict(import_hook_module);`
`121`	`121`	`// reference not stolen due to overload incref'ing for us.`
`122`	`122`	`Runtime.PyTuple_SetItem(args, 1, mod_dict);`
`123`		`- Runtime.PyObject_Call(exec, args, default);`
	`123`	`+ Runtime.PyObject_Call(exec, args, default).Dispose();`
`124`	`124`	`// Set as a sub-module of clr.`
`125`	`125`	`if(Runtime.PyModule_AddObject(ClrModuleReference, "loader", import_hook_module.DangerousGetAddress()) != 0)`
`126`	`126`	`{`