Skip to content
Merged
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
Add Namespaces to the import hook only through AddReference
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
  • Loading branch information
BadSingleton committed Jun 2, 2021
commit ff170e99b78ef411e54efd4d2d429799670d043f
15 changes: 0 additions & 15 deletions src/runtime/assemblymanager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ internal class AssemblyManager
// modified from event handlers below, potentially triggered from different .NET threads
private static ConcurrentQueue<Assembly> assemblies;
internal static List<string> pypath;

// Triggered when a new namespace is added to the namespaces dictionary
// public static event Action<string> namespaceAdded;

private AssemblyManager()
{
}
Expand Down Expand Up @@ -287,17 +283,6 @@ internal static void ScanAssembly(Assembly assembly)
if (ns != null)
{
namespaces[ns].TryAdd(assembly, string.Empty);
// try
// {
// namespaceAdded?.Invoke(ns);
// }
// catch (Exception e)
// {
// // For some reason, exceptions happening here does... nothing.
// // Even System.AccessViolationExceptions gets ignored.
// Console.WriteLine($"Namespace added callback failed with: {e}");
// throw;
// }
}

if (ns != null && t.IsGenericTypeDefinition)
Expand Down
15 changes: 2 additions & 13 deletions src/runtime/importhook.cs
Original file line number Diff line number Diff line change
Expand Up @@ -176,30 +176,21 @@ static void SetupNamespaceTracking()
newset.Dispose();
}

// AssemblyManager.namespaceAdded += OnNamespaceAdded;
// PythonEngine.AddShutdownHandler(() => AssemblyManager.namespaceAdded -= OnNamespaceAdded);
}

/// <summary>
/// Removes the set of available namespaces from the clr module and
/// removes the callback on the OnNamespaceAdded event.
/// Removes the set of available namespaces from the clr module.
/// </summary>
static void TeardownNameSpaceTracking()
{
// AssemblyManager.namespaceAdded -= OnNamespaceAdded;
// If the C# runtime isn't loaded, then there are no namespaces available
Runtime.PyDict_SetItemString(root.dict, availableNsKey, 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.

Why not remove the key entirely? You would not need to compare it to None below in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DotNetFinder relies on that key being present on the object. If I remove it in AddNamespace, I need to add a check over there instead.

}

public static void OnNamespaceAdded(string name)
public static void AddNamespace(string name)
{
Console.WriteLine(System.Environment.StackTrace);
Console.WriteLine("OnNamespaceAdded: acquiring");
Console.Out.Flush();
using (Py.GIL())
Copy link
Member

Choose a reason for hiding this comment

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

Are there scenarios when this method will be called without GIL acquired?
If it is a module method, can it be attributed with allowthreads=false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be possible. The method calling it now has the attribute

{
Console.WriteLine("OnNamespaceAdded: acquired");
Console.Out.Flush();
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
{
Expand All @@ -217,8 +208,6 @@ public static void OnNamespaceAdded(string name)
Runtime.XDecref(pyNs);
}
}
Console.WriteLine("OnNamespaceAdded: released");
Console.Out.Flush();
}


Expand Down
10 changes: 7 additions & 3 deletions src/runtime/moduleobject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ public static bool SuppressOverloads
public static Assembly AddReference(string name)
{
AssemblyManager.UpdatePath();
var origNs = AssemblyManager.GetNamespaces();
Assembly assembly = null;
assembly = AssemblyManager.FindLoadedAssembly(name);
if (assembly == null)
Expand All @@ -530,9 +531,12 @@ public static Assembly AddReference(string name)
// Classes that are not in a namespace needs an extra nudge to be found.
ImportHook.UpdateCLRModuleDict();

// Heavyhanded but otherwise we'd need a "addedSinceLastCall".
foreach(var ns in AssemblyManager.GetNamespaces()){
ImportHook.OnNamespaceAdded(ns);
// A bit heavyhanded, but we can't use the AssemblyManager's AssemblyLoadHandler
// method because it may be called from other threads, leading to deadlocks
// if it is called while Python code is executing.
var currNs = AssemblyManager.GetNamespaces().Except(origNs);
foreach(var ns in currNs){
ImportHook.AddNamespace(ns);
}
return assembly;
}
Expand Down