-
Notifications
You must be signed in to change notification settings - Fork 10
Modernize import hook #13
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: upstream
Are you sure you want to change the base?
Conversation
…ET interop New method ThrowLastAsClrException should be used everywhere instead of obsolete PythonException constructor. It automatically restores .NET exceptions, and applies codecs for Python exceptions. Traceback, PyVal and PyType are now stored and returned as PyObjects. PythonException now has InnerException set from its cause (e.g. __cause__ in Python, if any). PythonException.Restore no longer clears the exception instance. All helper methods were removed from public API surface.
…chInfo masking exception object
The new type indicates parameters of C API functions, that steal references to passed objects.
@@ -47,6 +48,7 @@ details about the cause of the failure | |||
- Incorrectly using a non-generic type with type parameters now produces a helpful Python error instead of throwing NullReferenceException | |||
- `import` may now raise errors with more detail than "No module named X" | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the extra whitespace
src/domain_tests/TestRunner.cs
Outdated
}, | ||
new TestCase | ||
{ | ||
// The C# code for this test doesn't matter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please expand a little more: We're testing that the import hook in Python reloads properly, so the C# code for this test doesn't matter.
(in which case why have have any code at all? Can you compile an empty string?)
src/runtime/assemblymanager.cs
Outdated
// For some reason, exceptions happening here does... nothing. | ||
// Even System.AccessViolationExceptions gets ignored. | ||
Console.WriteLine($"Namespace added callback failed with: {e}"); | ||
throw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why catch and rethrow? Just to get a log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The throw does nothing; the caller catches all exceptions. It might be a bug in the runtime (it might throw appropriately in mono), it might change in the future, so I'm rethrowing. I'm also logging because it took a long time to find why thing were borken further down the execution.
src/runtime/importhook.cs
Outdated
class DotNetFinder(importlib.abc.MetaPathFinder): | ||
|
||
def __init__(self): | ||
print('DotNetFinder init') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should have been removed
src/runtime/importhook.cs
Outdated
/// Return the clr python module (new reference) | ||
/// </summary> | ||
public static IntPtr GetCLRModule(IntPtr? fromList = null) | ||
static void SetupNamespaceTracking () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A header comment here would be nice, answering: what is namespace tracking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing substantial on the code; I think it would be nice to have some comments though in any new function you created.
Also added GetPythonThreadID
200ba3d
to
f502b8e
Compare
…net#1361) Better error messages for method argument mismatch, Python to managed value conversion, and other problems. - Converter.ToManaged obeys setError consistently - PyObject_Hash and tp_hash return nint - MakeGenericType and MakeGenericMethod have try/catch - RaiseTypeError sets __cause__ instead of changing the message string - PythonException.Normalize throws if an error is set - AggregateExceptions print better from Python - For an overloaded method, MethodBinder.Bind collects all binding errors with PythonExceptions as the inner.inner exceptions. - ExceptionClassObject.tp_str calls Exception.ToString() instead of Exception.Message (for justification see https://stackoverflow.com/questions/2176707/exception-message-vs-exception-tostring ) - Binding failures always have AggregateException as the cause, - Disabled test_method_parameters_change
…ers in Python, by returning the modified parameter values in a tuple. BREAKING: MethodBinder omits a void return type when returning a tuple of out parameters. DelegateManager unpacks a tuple of out parameters from Python (reversing the logic in MethodBinder) and sets the out parameters of the delegate.
This reverts commit 4684523.
* Return ModuleObject.pyHandle, do not convert. * Write domain tests generated code to file.
2860632
to
970a189
Compare
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
Use exclusively PyUnicode_DecodeUTF16 for .NET->Python string conversion
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?
pythonnet#727 pythonnet#1245 pythonnet#547 pythonnet#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.
AUTHORS
CHANGELOG