Add GetPythonThreadID and Interrupt methods in PythonEngine #1337

gpetrou · 2020-12-26T07:24:48Z

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.

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

src/embed_tests/TestInterrupt.cs

filmor · 2020-12-26T20:46:12Z

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.

filmor · 2020-12-26T20:48:01Z

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.

gpetrou · 2020-12-27T06:56:20Z

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?

src/runtime/pythonengine.cs

filmor · 2020-12-27T09:37:41Z

src/runtime/pythonengine.cs

+            if (Runtime.PyVersion >= new Version(3, 8))
+            {
+                dynamic threading = Py.Import("threading");
+                return threading.get_native_id();


Should get the GIL, I think.

Isn't the user supposed to do that when calling the method, as I did in the test?

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?

filmor · 2020-12-27T09:39:11Z

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.

gpetrou · 2020-12-31T09:18:43Z

@filmor can this be merged now?

src/runtime/runtime.cs

lostmsu · 2020-12-31T19:39:30Z

src/runtime/runtime.cs

@@ -2143,6 +2143,18 @@ internal static void Py_CLEAR(ref IntPtr ob)
        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
        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);


Why do you need overloads with uint and int? They are basically equivalent at bit level.

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?

@filmor on the binary side there's no difference. The caller, if necessary, can bitcast. Any thoughts?

I have removed the int and long declarations for now.

lostmsu · 2020-12-31T19:53:58Z

src/runtime/pythonengine.cs

+        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.");
+        }


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.

get_ident is a separate thing, the IDs are not compatible with the "remote raise".

@filmor are you sure? Documentation is very bad on this, but here's what I found:

get_ident appears to be returning PyThread_get_thread_ident
tstate->thread_id is also PyThread_get_thread_ident()

PyThreadState_SetAsyncExc finds thread by its tstate->thread_id

Using get_ident seems to work. What would you like me to do? Modify GetNativeThreadID method or remove it completely?

Remove it would be better.

@lostmsu You are right. But then we should still expose threading.get_ident or PyThread_get_thread_ident, no?

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.

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.

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

src/runtime/pythonengine.cs

lostmsu · 2021-01-04T22:59:02Z

CHANGELOG.md

@@ -10,6 +10,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].
 ### Added

 -   Ability to instantiate new .NET arrays using `Array[T](dim1, dim2, ...)` syntax
+-   Add GetNativeThreadID and Interrupt methods in PythonEngine


GetNativeThreadID should be gone now

filmor · 2021-01-11T20:39:20Z

src/runtime/pythonengine.cs

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


If we keep this, it should be renamed to GetPythonThreadId and doesn't this require the GIL in general?

Can you please explain why we need GIL here and not in the Interrupt method? GIL is used for both in the tests already.

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

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

related to pythonnet#1337

should resolve this failure: https://github.com/pythonnet/pythonnet/pull/1392/checks?check_run_id=1944113649 related to pythonnet#1337

should resolve this failure: https://github.com/pythonnet/pythonnet/pull/1392/checks?check_run_id=1944113649 related to #1337

gpetrou force-pushed the Interrupt branch 9 times, most recently from 9b35013 to d075db3 Compare December 26, 2020 16:34

filmor reviewed Dec 26, 2020

View reviewed changes

src/embed_tests/TestInterrupt.cs Outdated Show resolved Hide resolved

gpetrou force-pushed the Interrupt branch 3 times, most recently from 07b9655 to c3f7c78 Compare December 27, 2020 07:17

filmor reviewed Dec 27, 2020

View reviewed changes

src/runtime/pythonengine.cs Outdated Show resolved Hide resolved

filmor reviewed Dec 27, 2020

View reviewed changes

gpetrou force-pushed the Interrupt branch 8 times, most recently from fb52aa3 to 90a08b6 Compare December 31, 2020 09:08

lostmsu requested changes Dec 31, 2020

View reviewed changes

lostmsu requested changes Jan 4, 2021

View reviewed changes

gpetrou force-pushed the Interrupt branch 2 times, most recently from ce3d6b4 to 9b18f26 Compare January 8, 2021 07:00

filmor reviewed Jan 11, 2021

View reviewed changes

gpetrou added 3 commits January 21, 2021 07:37

Add Interrupt method in PythonEngine

8b848b3

Add GetNativeThreadID method in PythonEngine and different PyThreadSt…

d07cd05

…ate_SetAsyncExc calls for OS and Python version

Use get_ident and LLP64 and LP64 in DllImport names and ulong in publ…

f077caf

…ic methods and assert nativeThreadID value

gpetrou changed the title ~~Add Interrupt method in PythonEngine~~ Add GetPythonThreadID and Interrupt methods in PythonEngine Jan 21, 2021

gpetrou force-pushed the Interrupt branch 2 times, most recently from 6bcda67 to c455ee4 Compare January 21, 2021 09:00

Rename to GetPythonThreadID

8e7b7c6

gpetrou force-pushed the Interrupt branch from c455ee4 to 8e7b7c6 Compare January 21, 2021 09:04

lostmsu merged commit 5fd77b1 into pythonnet:master Jan 21, 2021

lostmsu added a commit to losttech/pythonnet that referenced this pull request Feb 21, 2021

made InterruptTest more robust

d261ec2

related to pythonnet#1337

lostmsu added a commit to losttech/pythonnet that referenced this pull request Feb 21, 2021

made InterruptTest more robust

9724205

should resolve this failure: https://github.com/pythonnet/pythonnet/pull/1392/checks?check_run_id=1944113649 related to pythonnet#1337

lostmsu added a commit to losttech/pythonnet that referenced this pull request Feb 21, 2021

made InterruptTest more robust

831fc85

should resolve this failure: https://github.com/pythonnet/pythonnet/pull/1392/checks?check_run_id=1944113649 related to pythonnet#1337

lostmsu mentioned this pull request Feb 21, 2021

Made InterruptTest more robust #1393

Merged

lostmsu added a commit to losttech/pythonnet that referenced this pull request Feb 21, 2021

made InterruptTest more robust

6c16787

should resolve this failure: https://github.com/pythonnet/pythonnet/pull/1392/checks?check_run_id=1944113649 related to pythonnet#1337

lostmsu added a commit that referenced this pull request Feb 21, 2021

made InterruptTest more robust

83328d2

should resolve this failure: https://github.com/pythonnet/pythonnet/pull/1392/checks?check_run_id=1944113649 related to #1337

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

Add GetPythonThreadID and Interrupt methods in PythonEngine #1337

Add GetPythonThreadID and Interrupt methods in PythonEngine #1337

Uh oh!

Conversation

gpetrou commented Dec 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Checklist

Uh oh!

Uh oh!

filmor commented Dec 26, 2020

Uh oh!

filmor commented Dec 26, 2020

Uh oh!

gpetrou commented Dec 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpetrou Dec 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor commented Dec 27, 2020

Uh oh!

gpetrou commented Dec 31, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gpetrou commented Dec 26, 2020 •

edited

Loading

gpetrou Dec 27, 2020 •

edited

Loading

filmor Dec 31, 2020 •

edited

Loading