NotificationsYou must be signed in to change notification settings
Fork749
Star5.1k

Modernize import hook#1369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

filmor merged 21 commits intopythonnet:masterfromUnity-Technologies:modernize-import-hook

Jun 18, 2021

Merged

Modernize import hook#1369

filmor merged 21 commits intopythonnet:masterfromUnity-Technologies:modernize-import-hook

Jun 18, 2021

Conversation

Copy link

Contributor

BadSingleton commentedJan 25, 2021

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

BadSingleton force-pushed themodernize-import-hook branch 2 times, most recently from200ba3d tof502b8eCompare

January 25, 2021 19:50

filmor reviewed

Jan 25, 2021

View reviewed changes

src/runtime/importhook.cs OutdatedShow resolvedHide resolved

benoithudson reviewed

Apr 13, 2021

View reviewed changes

src/runtime/importhook.cs OutdatedShow resolvedHide resolved

benoithudson reviewed

Apr 13, 2021

View reviewed changes

src/runtime/importhook.cs OutdatedShow resolvedHide resolved

benoithudson reviewed

Apr 13, 2021

View reviewed changes

src/runtime/importhook.cs Outdated

		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);

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

oh.. that should befinder_inst, not finder

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Same forPySys_GetObject - you can rename the original one toDangerous_PySys_GetObject to avoid refactoring other call sites.

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

benoithudson reviewed

Apr 13, 2021

View reviewed changes

src/runtime/importhook.cs Outdated

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

NIT: I'd replacenew BorrowedReference(IntPtr.Zero) with eitherBorrowedReference.Null or simplydefault.

benoithudson reviewed

Apr 13, 2021

View reviewed changes

src/runtime/importhook.cs Outdated

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'd keep separateIsNull andIsNone. Thought there's alreadyIsNone.

BadSingleton force-pushed themodernize-import-hook branch from0f1219b tof9a3974Compare

June 1, 2021 15:28

BadSingleton added15 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 commit4684523.

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 themodernize-import-hook branch from2860632 to970a189Compare

June 2, 2021 15:08

BadSingleton added2 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 becauseit may be called from other threads, leading to deadlocks if it iscalled while Python code is executing

lostmsu requested changes

Jun 3, 2021

View reviewed changes

Copy link

Member

lostmsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Only reviewed in regards to usage of references and exceptions.

@filmor should still sign off on functionality.

src/runtime/BorrowedReference.cs Outdated

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/runtime/importhook.cs Outdated

		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);

Copy link

Member

lostmsuJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'd add a propertyClrModuleReference that returns aBorrowedReference ofpy_clr_module, and then used it inPyDict_SetItemString overload, that works withBorrowedReferences to avoid usingDangerous* methods.

src/runtime/importhook.cs Outdated

		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);

Copy link

Member

lostmsuJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

pythonnet/src/runtime/runtime.cs

Line 996 in7d8f754

internalstaticIntPtrPyObject_Type(IntPtrop)

One can also be made forPyTuple_SetItem using the newStolenReference.

Copy link

Member

lostmsuJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/runtime/importhook.csShow resolvedHide resolved

src/runtime/importhook.cs Outdated

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

Copy link

Member

lostmsuJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

using var newset = ... would remove the need fortry ... finally ...

src/runtime/moduleobject.cs Outdated

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

Copy link

Member

lostmsuJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Frankly, I screwed upPyList_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?

Copy link

ContributorAuthor

BadSingletonJun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/runtime/moduleobject.cs

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

Copy link

Member

lostmsuJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why do you have to forbid threads here?

Copy link

ContributorAuthor

BadSingletonJun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/runtime/moduleobject.cs Outdated

Comment on lines 600 to 606

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

Copy link

Member

lostmsuJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

NIT:

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

src/runtime/moduleobject.csShow resolvedHide resolved

src/runtime/runtime.cs Outdated

		@@ -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)

Copy link

Member

lostmsuJun 3, 2021•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks likestolenObject parameter should be of typeStolenReference here and below (present in the latestmaster).

Copy link

ContributorAuthor

BadSingletonJun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

StolenReference here and below (present in the latest master)

🎉

Copy link

ContributorAuthor

BadSingletonJun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

lostmsuJun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link

Member

lostmsuJun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think that we should not useBorrowedReference 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 itIntPtr for now, and explain the behavior in the parameter doc.

Or alternatively, make a wrapper, that takes eitherBorrowedReference orStolenReference and handles refcounting internally, and keep the real PInvoke private.

Copy link

ContributorAuthor

BadSingletonJun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Review changes, update API usage

63de923

BadSingleton requested a review fromfilmor

June 8, 2021 19:51

BadSingleton requested a review fromlostmsu

June 8, 2021 20:05

lostmsu requested changes

Jun 10, 2021

View reviewed changes

src/runtime/converter.cs Outdated

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

Copy link

Member

lostmsuJun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You need to return a new reference here.

Copy link

Member

lostmsuJun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@BadSingleton what about this one?

Copy link

ContributorAuthor

BadSingletonJun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

My bad missed that one.

src/runtime/runtime.cs Outdated

		@@ -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)

Copy link

Member

lostmsuJun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Or alternatively, make a wrapper, that takes eitherBorrowedReference orStolenReference and handles refcounting internally, and keep the real PInvoke private.