Adds an unchecked version to get a BorrowedReference pointer #4

BadSingleton · 2020-09-15T18:33:22Z

And other code review changes.

And other code review changes

Even if netstandard lacks (for now) the necessary APIs for domain (un)loading, it is still possible to (un)load domains via the Mono C runtime.

In Runtime_data.cs

…e pointer" We Don't support python 2 anymore, but the CI machines may still be using it to build.

codecov-commenter · 2020-09-16T15:51:55Z

Codecov Report

Merging #4 into soft-shutdown will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           soft-shutdown       #4   +/-   ##
==============================================
  Coverage          86.25%   86.25%           
==============================================
  Files                  1        1           
  Lines                291      291           
==============================================
  Hits                 251      251           
  Misses                40       40

Flag	Coverage Δ
#setup_linux	`64.94% <ø> (ø)`
#setup_windows	`72.50% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12c0206...ff956e4. Read the comment docs.

lostmsu · 2020-09-24T16:17:13Z

src/runtime/runtime_data.cs

+            IntPtr capsule = PySys_GetObject("clr_data").DangerousGetAddressUnchecked();
+            if (capsule != IntPtr.Zero)
+            {
+                IntPtr oldData = PyCapsule_GetPointer(capsule, null);
+                PyMem_Free(oldData);
+                PyCapsule_SetPointer(capsule, IntPtr.Zero);
+            }


Now that you never own the pointer, you can use BorrowedReference and never call "Dangerous" methods on it. Make PyCapsule_GetPointer and PyCapsule_SetPointer take first argument as BorrowedReference.

Make PyCapsule_GetPointer and PyCapsule_SetPointer take first argument as BorrowedReference

I don't think this is semantically correct. These methods can operate on something other than BorrowedReferences

No, this is exactly right. The method borrows a capsule object in a similar way we use borrowed references. The second argument of course should stay as raw pointer.

Think of it this way: if you were to implement PyCapsule_GetPointer, you'd need a borrowed reference to the capsule in order to update it (as opposed to stealing or taking/giving up ownership).

In this use case, it is. However in the case where I create the new capsule (and receive a NewReference), and then set the capsule's pointer with PyCapsule_SetPointer, well.. it's a bit weird. But as I said in another reply, NewReference in implicitly castable to borrowed reference, which makes it transparent to the user.

lostmsu · 2020-09-24T16:17:55Z

src/runtime/runtime_data.cs

        private static void RestoreRuntimeDataImpl()
        {
-            IntPtr capsule = PySys_GetObject("clr_data").DangerousGetAddress();
+            IntPtr capsule = PySys_GetObject("clr_data").DangerousGetAddressUnchecked();


Same here, you don't need the raw address.

I do need it. PyCapsule_GetPointer at line 113 needs an IntPtr. Sure I could add an overload for PyCapsule_GetPointer, but I think what we really need is an implicit operator.

lostmsu · 2020-09-24T16:31:00Z

src/runtime/runtime_data.cs

-            }
-            capsule = PyCapsule_New(mem, null, IntPtr.Zero);
+            ClearCLRData();
+            IntPtr capsule = PyCapsule_New(mem, null, IntPtr.Zero);


Ideally, this should have been NewReference, and decref below should be Dispose

Also adds implicit IntPtr conversion operators to simplify their use.

lostmsu · 2020-09-24T18:42:31Z

src/runtime/BorrowedReference.cs

+        public static implicit operator IntPtr(in BorrowedReference self) => self.DangerousGetAddress();
+


This exposes dangerous shotgun as an innocent operator

lostmsu · 2020-09-24T18:42:42Z

src/runtime/NewReference.cs

            => new BorrowedReference(reference.pointer);

+        [Pure]
+        public static implicit operator IntPtr(in NewReference reference)


lostmsu · 2020-09-24T18:44:25Z

src/runtime/runtime.cs

+        /// <remark>
+        /// We need this method because BorrowedReference can be implicitly casted to IntPtr.
+        /// </remark>
+        internal static void XDecref(BorrowedReference op)
+        {
+            throw new InvalidOperationException("Cannot DecRef a borrowed reference.");
+        }
+
+        /// <remark>
+        /// We need this method because NewReference can be implicitly casted to IntPtr.
+        /// </remark>
+        internal static void XDecref(NewReference op)
+        {
+            op.Dispose();
+        }
+


No implicit casts. The types were created specifically to avoid freely mixing different kinds of ownership for PyObject*.

lostmsu · 2020-09-24T18:46:41Z

src/runtime/runtime.cs


        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
-        internal static extern IntPtr PyCapsule_New(IntPtr pointer, string name, IntPtr destructor);
+        internal static extern NewReference PyCapsule_New(IntPtr pointer, string name, IntPtr destructor);


Re: implicit conversions to IntPtr: If you find it hard to change all the places, that use PyCapsule_New to use NewReference avoiding its dangerous methods as much as possible, just keep this return type IntPtr here, and use NewReference at the call site.

Not that I find it hard, I got confused by how the BorrowedReference gets marshalled. Or more like how it could get marshalled without needing an overload.
Also, NewReference is implicitly castable to BorrowedReference which solves most issues. The naming still bugs me a bit, but if it works, it works.

lostmsu · 2020-09-24T18:47:25Z

src/runtime/runtime.cs

+        internal static extern NewReference PyCapsule_New(IntPtr pointer, string name, IntPtr destructor);

        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
        internal static extern IntPtr PyCapsule_GetPointer(IntPtr capsule, string name);


This should be IntPtr PyCapsule_GetPointer(BorrowedReference capsule, string name)

Or you can add an overload.

BadSingleton · 2020-09-25T15:42:32Z

travis-ci has one failure on an unrelated error:

CSC : error CS0009: Metadata file '/home/travis/.nuget/packages/microsoft.targetingpack.netframework.v4.5/1.0.1/lib/net45/System.Xml.dll' could not be opened -- Unexpected stream end. [/home/travis/build/amos402/pythonnet/src/testing/Python.Test.15.csproj]

lostmsu

Looks good to me. The new Dangerous method is not used anywhere though, so should not be added.

lostmsu · 2020-09-25T20:03:48Z

src/runtime/BorrowedReference.cs

+        /// <summary>
+        /// Gets a raw pointer to the Python object. Does not throw an exception
+        ///  if the pointer is null
+        /// </summary>
+        public IntPtr DangerousGetAddressUnchecked() => this.pointer;


NIT: I don't think you are actually using this anywhere

Indeed, it's not used anymore

BadSingleton and others added 5 commits September 15, 2020 14:31

Adds an unchecked version to get a BorrowedReference pointer

6a3cfc8

And other code review changes

Remove compile-time check on NETSTANDARD

98da1fa

Even if netstandard lacks (for now) the necessary APIs for domain (un)loading, it is still possible to (un)load domains via the Mono C runtime.

fixup! Remove compile-time check on NETSTANDARD

d7d44e8

Factor out the clearing of clr_data

6aa75c5

In Runtime_data.cs

Partially Revert "Adds an unchecked version to get a BorrowedReferenc…

2343f89

…e pointer" We Don't support python 2 anymore, but the CI machines may still be using it to build.

lostmsu reviewed Sep 24, 2020

View reviewed changes

More changes to use BorrowedReference and NewReference

cc6b8e4

Also adds implicit IntPtr conversion operators to simplify their use.

lostmsu suggested changes Sep 24, 2020

View reviewed changes

BadSingleton added 2 commits September 24, 2020 16:10

fixup! More changes to use BorrowedReference and NewReference

d5fcfa4

fixup! fixup! More changes to use BorrowedReference and NewReference

fa47957

lostmsu approved these changes Sep 25, 2020

View reviewed changes

Remove unused method

ff956e4

amos402 merged commit c7b134c into amos402:soft-shutdown Sep 29, 2020

BadSingleton deleted the soft-shutdown-review-comments-3 branch October 20, 2020 14:59

		public static implicit operator IntPtr(in BorrowedReference self) => self.DangerousGetAddress();

Adds an unchecked version to get a BorrowedReference pointer #4

Adds an unchecked version to get a BorrowedReference pointer #4

Uh oh!

Conversation

BadSingleton commented Sep 15, 2020

Uh oh!

codecov-commenter commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lostmsu Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsu Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BadSingleton commented Sep 25, 2020

Uh oh!

lostmsu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Sep 16, 2020 •

edited

Loading

lostmsu Sep 24, 2020 •

edited

Loading

lostmsu Sep 24, 2020 •

edited

Loading

lostmsu Sep 24, 2020 •

edited

Loading

lostmsu Sep 24, 2020 •

edited

Loading