Add GetPythonThreadID and Interrupt methods in PythonEngine by gpetrou · Pull Request #1337 · pythonnet/pythonnet
What does this implement/fix? Explain your changes.
Fixes #766 by adding an Interrupt method in PythonEngine class.
Checklist
Check all those that are applicable and complete.
Copying my comment from #1333:
- This requires a test (done)
- Strictly, the thread-id attribute is unsigned only for Python >= 3.7 and we still support 3.6
- Also, since the original type is long, the "correct" type is (U)IntPtr on everything but Windows (Not quite sure how far we have to go there compatibility-wise, the current implementation could be fine for the usual thread IDs)
- Maybe we should just expose the API function directly (i.e. RaiseInPythonThread on Exception objects or so) and make Interrupt use that
Additional comments on the current state: The helper API to get the current thread id should be in the library and should use threading.get_native_id() on Python >=3.8.
It would also be good if you'd just use normal commits now without force-pushing a single commit over and over, otherwise it becomes very difficult to follow your changes. We can (and will) squash this in the end.
Please note that I am very unfamiliar with the codebase, so I will need more explanations from you. With regards to:
2. I added a version check. Is this what you mean?
3. I added different calls per OS and version. Is this what you mean?
4. I don't understand what that means. Can you elaborate? Is this still needed?
| if (Runtime.PyVersion >= new Version(3, 8)) | ||
| { | ||
| dynamic threading = Py.Import("threading"); | ||
| return threading.get_native_id(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should get the GIL, I think.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the user supposed to do that when calling the method, as I did in the test?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem to be working on Ubuntu. Are you sure that this should work? Looking in CPython tests I see that get_ident is used. I don't see any benefit in adding the above lines. Should I just remove them?
This goes in the right direction :)
Regarding 2.: Yes, that's what I meant.
Regarding 3.: This looks better, but the Windows type is uint and int. .NET's int is always 32bit, and on Windows long is also 32bit on both x86 and x64.
| internal static extern int Py_AddPendingCall(IntPtr func, IntPtr arg); | ||
|
|
||
| [DllImport(_PythonDll, EntryPoint = "PyThreadState_SetAsyncExc", CallingConvention = CallingConvention.Cdecl)] | ||
| internal static extern int PyThreadState_SetAsyncExc37Windows(uint id, IntPtr exc); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need overloads with uint and int? They are basically equivalent at bit level.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them because of "Strictly, the thread-id attribute is unsigned only for Python >= 3.7 and we still support 3.6" comment above. Do you want me to remove them?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filmor on the binary side there's no difference. The caller, if necessary, can bitcast. Any thoughts?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the int and long declarations for now.
Comment on lines +583 to +578
| public static ulong GetNativeThreadID() | ||
| { | ||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | ||
| { | ||
| return GetCurrentThreadId(); | ||
| } | ||
|
|
||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) | ||
| { | ||
| return pthread_selfLinux(); | ||
| } | ||
|
|
||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) | ||
| { | ||
| return pthread_selfOSX(); | ||
| } | ||
|
|
||
| throw new InvalidOperationException("Could not retrieve native thread ID."); | ||
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this belongs to PythonEngine. From what I read, for the test you might be able to use threading.get_ident() from Python thread module.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_ident is a separate thing, the IDs are not compatible with the "remote raise".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using get_ident seems to work. What would you like me to do? Modify GetNativeThreadID method or remove it completely?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it would be better.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lostmsu You are right. But then we should still expose threading.get_ident or PyThread_get_thread_ident, no?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? The user can do what tests do now - e.g. call Py.Import("threading").Invoke("get_ident").
@gpetrou It might make sense to mention threading.get_ident() in the summary of XML doc for the Interrupt method.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting mixed signals between the two of you :) Who is the BDFL for Python.NET? I added the get_ident usage in GetNativeThreadID for now.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gpetrou @filmor well, I am voting for removal, as it is one less thing to support with potential multiple meanings, and having nothing to do with Python embedding on itself.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ### Added | ||
|
|
||
| - Ability to instantiate new .NET arrays using `Array[T](dim1, dim2, ...)` syntax | ||
| - Add GetNativeThreadID and Interrupt methods in PythonEngine |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetNativeThreadID should be gone now
| @@ -582,28 +573,8 @@ public static void Exec(string code, IntPtr? globals = null, IntPtr? locals = nu | |||
| /// <returns>The native thread ID.</returns> | |||
| public static ulong GetNativeThreadID() | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep this, it should be renamed to GetPythonThreadId and doesn't this require the GIL in general?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why we need GIL here and not in the Interrupt method? GIL is used for both in the tests already.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you are right, all of the other functions in here require the GIL to be taken explicitly as well. I'm not terribly happy about this, but this should be fine then. The dynamic here should not be required, just use InvokeMethod:
var threading = ...;
return threading.InvokeMethod("get_ident");
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And renaming would still be nice, sorry for the back and forth, I misread the docs.
…ate_SetAsyncExc calls for OS and Python version
…ic methods and assert nativeThreadID value
gpetrou
changed the title
Add Interrupt method in PythonEngine
Add GetPythonThreadID and Interrupt methods in PythonEngine
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters