Skip to content

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Jun 18, 2021
Merged

Modernize import hook #1369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a321daa
(WIP) modernize the import hook
BadSingleton Jan 15, 2021
279b535
Add the loaded namespaces tracking
BadSingleton Jan 15, 2021
f92e95b
cleanup and changelog entry
BadSingleton Jan 15, 2021
afffc18
Fix a bug where clr wasn't in sys.modules after reload
BadSingleton Jan 18, 2021
d821c0f
Further refinements to setattr logic on ModuleObjects
BadSingleton Jan 19, 2021
e469a8a
fixups, add docs
BadSingleton Jan 20, 2021
685b972
merge fixup
BadSingleton Mar 4, 2021
be81364
(WIP) import hook in the pytohn module
BadSingleton Mar 10, 2021
73958ed
Revert "(WIP) import hook in the pytohn module"
BadSingleton Apr 12, 2021
e71a0ef
Import hook as a module, take 2
BadSingleton Apr 12, 2021
2af066d
fixup! Merge remote-tracking branch 'origin/master' into modernize-im…
BadSingleton Apr 12, 2021
bb490bf
fixup! fixup! Merge remote-tracking branch 'origin/master' into moder…
BadSingleton Apr 12, 2021
31ea876
Create a clr.loader module
BadSingleton Jun 1, 2021
c02d5c6
Test to test if the test passes
BadSingleton Jun 2, 2021
970a189
fix new exception usage
BadSingleton Jun 2, 2021
059605b
kick the build because I can't repro
BadSingleton Jun 2, 2021
ff170e9
Add Namespaces to the import hook only through AddReference
BadSingleton Jun 2, 2021
63de923
Review changes, update API usage
BadSingleton Jun 8, 2021
624f7e3
Merge remote-tracking branch 'upstream/master' into modernize-import-…
BadSingleton Jun 14, 2021
bd7e745
make PyModule_AddObject in line with CPython
BadSingleton Jun 14, 2021
46a85fe
take care of stragglers
BadSingleton Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
merge fixup
  • Loading branch information
@BadSingleton
BadSingleton committed Jun 2, 2021
commit 685b972001d2186e60d41f7a42959f74e6e8a5e0
1 change: 1 addition & 0 deletions src/embed_tests/TestPythonException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ public void TestPythonException_Normalize_ThrowsWhenErrorSet()
var pythonException = PythonException.FetchCurrentRaw();
Exceptions.SetError(Exceptions.TypeError, "Another error");
Assert.Throws<InvalidOperationException>(() => pythonException.Normalize());
Exceptions.Clear();
}
}
}
35 changes: 15 additions & 20 deletions src/runtime/importhook.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ internal static void RestoreRuntimeData(RuntimeDataStorage storage)
storage.GetValue("py_clr_module", out py_clr_module);
var rootHandle = storage.GetValue<IntPtr>("root");
root = (CLRModule)ManagedType.GetManagedObject(rootHandle);
IntPtr dict = Runtime.PyImport_GetModuleDict();
Runtime.PyDict_SetItemString(dict, "CLR", py_clr_module);
Runtime.PyDict_SetItemString(dict, "clr", py_clr_module);
BorrowedReference dict = Runtime.PyImport_GetModuleDict();
Runtime.PyDict_SetItemString(dict.DangerousGetAddress(), "CLR", py_clr_module);
Runtime.PyDict_SetItemString(dict.DangerousGetAddress(), "clr", py_clr_module);
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 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.

SetupNamespaceTracking();
}

Expand All @@ -122,15 +122,15 @@ internal static void RestoreRuntimeData(RuntimeDataStorage storage)
/// </summary>
static void SetupNamespaceTracking ()
{
var newset = Runtime.PySet_New(IntPtr.Zero);
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 a using 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 replace new BorrowedReference(IntPtr.Zero) with either BorrowedReference.Null or simply default.

try
{
foreach (var ns in AssemblyManager.GetNamespaces())
{
var pyNs = Runtime.PyString_FromString(ns);
try
{
if(Runtime.PySet_Add(newset, pyNs) != 0)
if(Runtime.PySet_Add(newset, new BorrowedReference(pyNs)) != 0)
{
throw new PythonException();
}
Expand All @@ -141,14 +141,14 @@ static void SetupNamespaceTracking ()
}
}

if(Runtime.PyDict_SetItemString(root.dict, availableNsKey, newset) != 0)
if(Runtime.PyDict_SetItemString(root.dict, availableNsKey, newset.DangerousGetAddress()) != 0)
{
throw new PythonException();
}
}
finally
{
Runtime.XDecref(newset);
newset.Dispose();
}

AssemblyManager.namespaceAdded += OnNamespaceAdded;
Expand All @@ -163,17 +163,13 @@ static void TeardownNameSpaceTracking()
{
AssemblyManager.namespaceAdded -= OnNamespaceAdded;
// If the C# runtime isn't loaded, then there is no namespaces available
if ((Runtime.PyDict_DelItemString(root.dict, availableNsKey) != 0) &&
if ((Runtime.PyDict_DelItemString(new BorrowedReference(root.dict), availableNsKey) != 0) &&
(Exceptions.ExceptionMatches(Exceptions.KeyError)))
{
// Trying to remove a key that's not in the dictionary
// raises an error. We don't care about it.
Runtime.PyErr_Clear();
}
else if (Exceptions.ErrorOccurred())
{
throw new PythonException();
}
}

static void OnNamespaceAdded (string name)
Expand All @@ -183,10 +179,10 @@ static void OnNamespaceAdded (string name)
var pyNs = Runtime.PyString_FromString(name);
Copy link
Member

Choose a reason for hiding this comment

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

NIT: using var pyNs = NewReference.DangerousFromPoiner(Runtime.PyString_FromString(name)) would remove both try ... finally ... and a cast for PySet_Add

try
{
var nsSet = Runtime.PyDict_GetItemString(root.dict, availableNsKey);
if (nsSet != IntPtr.Zero)
var nsSet = Runtime.PyDict_GetItemString(new BorrowedReference(root.dict), availableNsKey);
if (!nsSet.IsNull)
{
if(Runtime.PySet_Add(nsSet, pyNs) != 0)
if(Runtime.PySet_Add(nsSet, new BorrowedReference(pyNs)) != 0)
{
throw new PythonException();
}
Expand All @@ -211,16 +207,15 @@ internal static void UpdateCLRModuleDict()
// update the module dictionary with the contents of the root dictionary
root.LoadNames();
BorrowedReference py_mod_dict = Runtime.PyModule_GetDict(ClrModuleReference);
using (var clr_dict = Runtime.PyObject_GenericGetDict(root.ObjectReference))
{
Runtime.PyDict_Update(py_mod_dict, clr_dict);
}
using var clr_dict = Runtime.PyObject_GenericGetDict(root.ObjectReference);

Runtime.PyDict_Update(py_mod_dict, clr_dict);
}

/// <summary>
/// Return the clr python module (new reference)
/// </summary>
public static unsafe NewReference GetCLRModule(BorrowedReference fromList = default)
public static unsafe NewReference GetCLRModule()
{
UpdateCLRModuleDict();
Runtime.XIncref(py_clr_module);
Expand Down