Modernize import hook #1369

BadSingleton · 2021-01-25T19:38:03Z

What does this implement/fix? Explain your changes.

This replaces the current import hook replacement with a MetaPathFinder, letting python handle non-CLR module import.
Does this close any currently open issues?

#727 #1245 #547 #111 and probably others.
Any other comments?

This fix also helps working around a mono runtime behaviour where a mono owns all threads that execute .NET code.
Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

src/runtime/importhook.cs

benoithudson · 2021-04-13T15:06:41Z

src/runtime/importhook.cs

+            var finder_inst = Runtime.PyDict_GetItemString(mod_dict, "finder_inst").DangerousGetAddressOrNull();
+            Runtime.XIncref(finder);
+            var metapath = Runtime.PySys_GetObject("meta_path");
+            Runtime.PyList_Append(metapath, finder);


These four lines look fishy: you don't use finder_inst and you are doing an incref on finder for no obvious reason.

oh.. that should be finder_inst, not finder

BTW, it makes sense to add an overload for PyList_Append with proper reference types, then avoid manual refcounting.

Same for PySys_GetObject - you can rename the original one to Dangerous_PySys_GetObject to avoid refactoring other call sites.

This is a NIT though. Should come up with a better plan to switch everything.

benoithudson · 2021-04-13T15:08:16Z

src/runtime/importhook.cs

-            BorrowedReference fromList = default;
-            var fromlist = false;
-            if (num_args >= 4)
+            var newset = Runtime.PySet_New(new BorrowedReference(IntPtr.Zero));


You Dispose in a finally; should this be a using var ?

NIT: I'd replace new BorrowedReference(IntPtr.Zero) with either BorrowedReference.Null or simply default.

benoithudson · 2021-04-13T15:10:45Z

src/runtime/importhook.cs

                {
-                    if (IsLoadAll(fromList))
+                    var nsSet = Runtime.PyDict_GetItemString(new BorrowedReference(root.dict), availableNsKey);
+                    if (!nsSet.IsNull || nsSet.DangerousGetAddress() != Runtime.PyNone)


Should BorrowedReference have an IsNullOrNone property? It seems likely to be a common idiom.

I'd keep separate IsNull and IsNone. Thought there's already IsNone.

Implement a meta path loader instead

This reverts commit 4684523.

…port-hook

…nize-import-hook

* Return ModuleObject.pyHandle, do not convert. * Write domain tests generated code to file.

Don't piggyback on AssemblyManager's AssemblyLoadHandler method because it may be called from other threads, leading to deadlocks if it is called while Python code is executing

lostmsu

Only reviewed in regards to usage of references and exceptions.

@filmor should still sign off on functionality.

lostmsu · 2021-06-03T02:57:09Z

src/runtime/BorrowedReference.cs

@@ -9,6 +9,7 @@ namespace Python.Runtime
    {
        readonly IntPtr pointer;
        public bool IsNull => this.pointer == IntPtr.Zero;
+        public bool IsNone => this.pointer == Runtime.PyNone;


This should not be directly on BorrowedReference. None is a concept of the currently running runtime, and BorrowedReference might be a valid non-None reference from the previous runtime incarnation.

lostmsu · 2021-06-03T03:00:52Z

src/runtime/importhook.cs

            storage.GetValue("py_clr_module", out py_clr_module);
            var rootHandle = storage.GetValue<IntPtr>("root");
            root = (CLRModule)ManagedType.GetManagedObject(rootHandle);
+            BorrowedReference dict = Runtime.PyImport_GetModuleDict();
+            Runtime.PyDict_SetItemString(dict.DangerousGetAddress(), "clr", py_clr_module);


I'd add a property ClrModuleReference that returns a BorrowedReference of py_clr_module, and then used it in PyDict_SetItemString overload, that works with BorrowedReferences to avoid using Dangerous* methods.

lostmsu · 2021-06-03T03:08:39Z

src/runtime/importhook.cs

+            var mod_dict = Runtime.PyModule_GetDict(import_hook_module);
+            Runtime.XIncref(mod_dict.DangerousGetAddress());
+            Runtime.PyTuple_SetItem(args.DangerousGetAddress(), 1, mod_dict.DangerousGetAddress());
+            Runtime.PyObject_Call(exec.DangerousGetAddress(), args.DangerousGetAddress(), IntPtr.Zero);


It is better to make an overload with the new reference types when possible without rewriting existing code, than to call Dangerous* methods. See for example

pythonnet/src/runtime/runtime.cs

Line 996 in 7d8f754

internal static IntPtr PyObject_Type(IntPtr op)

One can also be made for PyTuple_SetItem using the new StolenReference.

This applies to a few Runtime methods across the PR. Basically anything where you can add a new overload.

src/runtime/importhook.cs

lostmsu · 2021-06-03T03:13:27Z

src/runtime/importhook.cs

+        /// </summary>
+        static void SetupNamespaceTracking()
+        {
+            var newset = Runtime.PySet_New(default);


using var newset = ... would remove the need for try ... finally ...

lostmsu · 2021-06-03T03:25:20Z

src/runtime/moduleobject.cs

+                    var pyname = Runtime.PyString_FromString(name);
+                    try
+                    {
+                        if (Runtime.PyList_Append(new BorrowedReference(__all__), pyname) != 0)


Frankly, I screwed up PyList_Append. Its only overload schizophrenically mixes references and raw pointers. Can you change it to either have two overloads with only references and with only pointers, or just one that takes only references?

There are only a few calls to PyList_Append throughout the code base, I changed all of them to use only references.

lostmsu · 2021-06-03T03:26:42Z

src/runtime/moduleobject.cs

+        /// This is needed because the import mechanics need
+        /// to set a few attributes
+        /// </summary>
+        [ForbidPythonThreads]


Why do you have to forbid threads here?

Because there's a call to Runtime.PyDict_SetItem. I might be missing something and this is always called from CPython and the GIL is guaranteed to be held.

lostmsu · 2021-06-03T03:27:51Z

src/runtime/moduleobject.cs

+            ModuleObject mod = null;
+            using (var modname = spec.GetAttr("name"))
+            {
+                mod = ImportHook.Import(modname.ToString());
+            }
+            return mod;
+        }


NIT:

using var modname = ...; return ...;

src/runtime/moduleobject.cs

lostmsu · 2021-06-03T03:29:07Z

src/runtime/runtime.cs

@@ -1944,6 +1944,11 @@ internal static string PyModule_GetFilename(IntPtr module)


        internal static IntPtr PyImport_Import(IntPtr name) => Delegates.PyImport_Import(name);
+        internal static int PyModule_AddObject(BorrowedReference module, string name, BorrowedReference stolenObject)


Looks like stolenObject parameter should be of type StolenReference here and below (present in the latest master).

StolenReference here and below (present in the latest master)

🎉

Although I'm reviewing the semantics, and this function only steals on success, so by using StolenReference, we leak on failure.

Dang you are right. Why did they not make it consistent? PyTuple_SetItem for example does steal even when there's an error. ugh...

Maybe we should make a wrapper, that always steals?

I think that we should not use BorrowedReference for the last parameter here. It makes false suggestion, that signature is valid, and borrowed reference is always enough when it is really not. I'd keep it IntPtr for now, and explain the behavior in the parameter doc.

Or alternatively, make a wrapper, that takes either BorrowedReference or StolenReference and handles refcounting internally, and keep the real PInvoke private.

I'm leaning towards the IntPtr parameter to be as "consistant" as possible with the CPython API.

lostmsu · 2021-06-09T08:45:12Z

src/runtime/converter.cs

+            // pyHandle as is, do not convert.
+            if (value is ModuleObject modobj)
+            {
+                return modobj.pyHandle;


You need to return a new reference here.

@BadSingleton what about this one?

My bad missed that one.

lostmsu · 2021-06-10T04:56:26Z

src/runtime/runtime.cs

@@ -1944,6 +1944,11 @@ internal static string PyModule_GetFilename(IntPtr module)


        internal static IntPtr PyImport_Import(IntPtr name) => Delegates.PyImport_Import(name);
+        internal static int PyModule_AddObject(BorrowedReference module, string name, BorrowedReference stolenObject)


I think that we should not use BorrowedReference for the last parameter here. It makes false suggestion, that signature is valid, and borrowed reference is always enough when it is really not. I'd keep it IntPtr for now, and explain the behavior in the parameter doc.

Or alternatively, make a wrapper, that takes either BorrowedReference or StolenReference and handles refcounting internally, and keep the real PInvoke private.

…hook

lostmsu

@filmor I'm good with the code. Please see if you are OK with the approach.

filmor · 2021-06-16T05:29:39Z

I'll review this today.

hiaselhans · 2021-06-17T13:27:39Z

great news! :)

BadSingleton force-pushed the modernize-import-hook branch 2 times, most recently from 200ba3d to f502b8e Compare January 25, 2021 19:50

filmor reviewed Jan 25, 2021

View reviewed changes

benoithudson reviewed Apr 13, 2021

View reviewed changes

src/runtime/importhook.cs Outdated Show resolved Hide resolved

benoithudson reviewed Apr 13, 2021

View reviewed changes

src/runtime/importhook.cs Outdated Show resolved Hide resolved

benoithudson reviewed Apr 13, 2021

View reviewed changes

BadSingleton force-pushed the modernize-import-hook branch from 0f1219b to f9a3974 Compare June 1, 2021 15:28

BadSingleton added 15 commits June 2, 2021 10:55

(WIP) modernize the import hook

a321daa

Implement a meta path loader instead

Add the loaded namespaces tracking

279b535

cleanup and changelog entry

f92e95b

Fix a bug where clr wasn't in sys.modules after reload

afffc18

Further refinements to setattr logic on ModuleObjects

d821c0f

fixups, add docs

e469a8a

merge fixup

685b972

(WIP) import hook in the pytohn module

be81364

Revert "(WIP) import hook in the pytohn module"

73958ed

This reverts commit 4684523.

Import hook as a module, take 2

e71a0ef

fixup! Merge remote-tracking branch 'origin/master' into modernize-im…

2af066d

…port-hook

fixup! fixup! Merge remote-tracking branch 'origin/master' into moder…

bb490bf

…nize-import-hook

Create a clr.loader module

31ea876

* Return ModuleObject.pyHandle, do not convert. * Write domain tests generated code to file.

Test to test if the test passes

c02d5c6

fix new exception usage

970a189

BadSingleton force-pushed the modernize-import-hook branch from 2860632 to 970a189 Compare June 2, 2021 15:08

BadSingleton added 2 commits June 2, 2021 13:59

kick the build because I can't repro

059605b

Add Namespaces to the import hook only through AddReference

ff170e9

Don't piggyback on AssemblyManager's AssemblyLoadHandler method because it may be called from other threads, leading to deadlocks if it is called while Python code is executing

lostmsu requested changes Jun 3, 2021

View reviewed changes

Review changes, update API usage

63de923

BadSingleton requested a review from filmor June 8, 2021 19:51

BadSingleton requested a review from lostmsu June 8, 2021 20:05

lostmsu requested changes Jun 10, 2021

View reviewed changes

BadSingleton added 3 commits June 14, 2021 09:36

Merge remote-tracking branch 'upstream/master' into modernize-import-…

624f7e3

…hook

make PyModule_AddObject in line with CPython

bd7e745

take care of stragglers

46a85fe

lostmsu approved these changes Jun 15, 2021

View reviewed changes

filmor approved these changes Jun 18, 2021

View reviewed changes

filmor merged commit ea61b03 into pythonnet:master Jun 18, 2021

filmor mentioned this pull request Jul 13, 2021

embbed in c#,can't not import dotnet namespace of the host. #1489

Closed

filmor mentioned this pull request Jul 27, 2022

Import does not work from Cython #1890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize import hook #1369

Modernize import hook #1369

BadSingleton commented Jan 25, 2021

benoithudson Apr 13, 2021

BadSingleton Apr 13, 2021

lostmsu Apr 14, 2021

benoithudson Apr 13, 2021

lostmsu Apr 14, 2021

benoithudson Apr 13, 2021

lostmsu Apr 14, 2021

lostmsu left a comment

lostmsu Jun 3, 2021

lostmsu Jun 3, 2021

lostmsu Jun 3, 2021

lostmsu Jun 3, 2021

lostmsu Jun 3, 2021

lostmsu Jun 3, 2021

BadSingleton Jun 8, 2021

lostmsu Jun 3, 2021

BadSingleton Jun 8, 2021

lostmsu Jun 3, 2021

lostmsu Jun 3, 2021 •

edited

Loading

BadSingleton Jun 8, 2021

BadSingleton Jun 8, 2021

lostmsu Jun 9, 2021

lostmsu Jun 10, 2021

BadSingleton Jun 14, 2021

lostmsu Jun 9, 2021

lostmsu Jun 14, 2021

BadSingleton Jun 15, 2021

lostmsu Jun 10, 2021

lostmsu left a comment

filmor commented Jun 16, 2021

hiaselhans commented Jun 17, 2021

		@@ -1944,6 +1944,11 @@ internal static string PyModule_GetFilename(IntPtr module)


		internal static IntPtr PyImport_Import(IntPtr name) => Delegates.PyImport_Import(name);
		internal static int PyModule_AddObject(BorrowedReference module, string name, BorrowedReference stolenObject)

Modernize import hook #1369

Modernize import hook #1369

Conversation

BadSingleton commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lostmsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lostmsu Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lostmsu left a comment

Choose a reason for hiding this comment

filmor commented Jun 16, 2021

hiaselhans commented Jun 17, 2021

lostmsu Jun 3, 2021 •

edited

Loading