NotificationsYou must be signed in to change notification settings
Fork768
Star5.4k

Add GetPythonThreadID and Interrupt methods in PythonEngine#1337

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

lostmsu merged 4 commits intopythonnet:masterfromgpetrou:Interrupt

Jan 21, 2021

Merged

Add GetPythonThreadID and Interrupt methods in PythonEngine#1337

lostmsu merged 4 commits intopythonnet:masterfromgpetrou:Interrupt

Jan 21, 2021

Conversation

Copy link

Contributor

gpetrou commentedDec 26, 2020•
edited
Loading

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 toAUTHORS
Updated theCHANGELOG

gpetrou force-pushed theInterrupt branch 9 times, most recently from9b35013 tod075db3Compare

December 26, 2020 16:34

filmor reviewed

Dec 26, 2020

View reviewed changes

src/embed_tests/TestInterrupt.cs OutdatedShow resolvedHide resolved

Copy link

Member

filmor commentedDec 26, 2020

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 usethreading.get_native_id() on Python >=3.8.

Copy link

Member

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.

Copy link

ContributorAuthor

gpetrou commentedDec 27, 2020

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?

gpetrou force-pushed theInterrupt branch 3 times, most recently from07b9655 toc3f7c78Compare

December 27, 2020 07:17

filmor reviewed

Dec 27, 2020

View reviewed changes

src/runtime/pythonengine.cs OutdatedShow resolvedHide resolved

filmor reviewed

Dec 27, 2020

View reviewed changes

src/runtime/pythonengine.cs Outdated

		if(Runtime.PyVersion>=newVersion(3,8))
		{
		dynamicthreading=Py.Import("threading");
		returnthreading.get_native_id();

Copy link

Member

filmorDec 27, 2020

Choose a reason for hiding this comment

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

Should get theGIL, I think.

Copy link

ContributorAuthor

gpetrouDec 27, 2020

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?

Copy link

ContributorAuthor

gpetrouDec 27, 2020•
edited
Loading

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?

Copy link

Member

filmor commentedDec 27, 2020

This goes in the right direction :)

Regarding 2.: Yes, that's what I meant.
Regarding 3.: This looks better, but the Windows type isuint andint. .NET'sint is always 32bit, and on Windowslong is also 32bit on both x86 and x64.

gpetrou force-pushed theInterrupt branch 8 times, most recently fromfb52aa3 to90a08b6Compare

December 31, 2020 09:08

Copy link

ContributorAuthor

gpetrou commentedDec 31, 2020

@filmor can this be merged now?

lostmsu requested changes

Dec 31, 2020

View reviewed changes

src/runtime/runtime.csShow resolvedHide resolved

src/runtime/runtime.cs Outdated

		internalstaticexternint Py_AddPendingCall(IntPtr func, IntPtr arg);

		[DllImport(_PythonDll, EntryPoint= "PyThreadState_SetAsyncExc", CallingConvention= CallingConvention.Cdecl)]
		internalstaticexternint PyThreadState_SetAsyncExc37Windows(uint id, IntPtr exc);

Copy link

Member

lostmsuDec 31, 2020

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 withuint andint? They are basically equivalent at bit level.

Copy link

ContributorAuthor

gpetrouJan 1, 2021

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?

Copy link

Member

lostmsuJan 1, 2021

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?

Copy link

ContributorAuthor

gpetrouJan 8, 2021

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.

src/runtime/pythonengine.cs Outdated

Comment on lines 583 to 578

		publicstaticulongGetNativeThreadID()
		{
		if(RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
		{
		returnGetCurrentThreadId();
		}

		if(RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
		{
		returnpthread_selfLinux();
		}

		if(RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
		{
		returnpthread_selfOSX();
		}

		thrownewInvalidOperationException("Could not retrieve native thread ID.");
		}

Copy link

Member

lostmsuDec 31, 2020

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 toPythonEngine. From what I read, for the test you might be able to usethreading.get_ident() from Pythonthread module.

Copy link

Member

filmorDec 31, 2020•
edited
Loading

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

Copy link

Member

lostmsuJan 1, 2021

Choose a reason for hiding this comment

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

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

get_identappears to be returningPyThread_get_thread_ident
tstate->thread_idis alsoPyThread_get_thread_ident()

PyThreadState_SetAsyncExcfinds thread by itststate->thread_id

Copy link

ContributorAuthor

gpetrouJan 2, 2021

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?

Copy link

Member

lostmsuJan 4, 2021

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.

Copy link

Member

filmorJan 5, 2021

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 exposethreading.get_ident orPyThread_get_thread_ident, no?

Copy link

Member

lostmsuJan 5, 2021

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. callPy.Import("threading").Invoke("get_ident").

@gpetrou It might make sense to mentionthreading.get_ident() in the summary of XML doc for theInterrupt method.

Copy link

ContributorAuthor

gpetrouJan 8, 2021

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.

Copy link

Member

lostmsuJan 8, 2021

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.

Copy link

Member

lostmsuJan 11, 2021

Choose a reason for hiding this comment

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

@filmor ?

src/runtime/pythonengine.cs OutdatedShow resolvedHide resolved

lostmsu requested changes

Jan 4, 2021

View reviewed changes

CHANGELOG.md Outdated

		###Added

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

Copy link

Member

lostmsuJan 4, 2021

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

gpetrou force-pushed theInterrupt branch 2 times, most recently fromce3d6b4 to9b18f26Compare

January 8, 2021 07:00

filmor reviewed

Jan 11, 2021

View reviewed changes

src/runtime/pythonengine.cs Outdated

		/// Gets the native thread ID.
		/// </summary>
		/// <returns>The native thread ID.</returns>
		publicstaticulongGetNativeThreadID()

Copy link

Member

filmorJan 11, 2021

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 toGetPythonThreadId and doesn't this require the GIL in general?

Copy link

ContributorAuthor

gpetrouJan 13, 2021

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.

Copy link

Member

filmorJan 20, 2021

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 theGIL to be taken explicitly as well. I'm not terribly happy about this, but this should be fine then. Thedynamic here should not be required, just useInvokeMethod:

var threading = ...;return threading.InvokeMethod("get_ident");

Copy link

Member

filmorJan 20, 2021

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.

gpetrou added3 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 theInterrupt branch 2 times, most recently from6bcda67 toc455ee4Compare

January 21, 2021 09:00

Rename to GetPythonThreadID

8e7b7c6

gpetrou force-pushed theInterrupt branch fromc455ee4 to8e7b7c6Compare

January 21, 2021 09:04

lostmsu merged commit5fd77b1 intopythonnet: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 topythonnet#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=1944113649related topythonnet#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=1944113649related topythonnet#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=1944113649related topythonnet#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=1944113649related to#1337

Labels

None yet

Movatterモバイル変換

Add GetPythonThreadID and Interrupt methods in PythonEngine#1337

Add GetPythonThreadID and Interrupt methods in PythonEngine#1337

Uh oh!

Conversation

gpetrou commentedDec 26, 2020• editedLoading 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 commentedDec 26, 2020

Uh oh!

filmor commentedDec 26, 2020

Uh oh!

gpetrou commentedDec 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpetrouDec 27, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmor commentedDec 27, 2020

Uh oh!

gpetrou commentedDec 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!

filmorDec 31, 2020• editedLoading 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gpetrou commentedDec 26, 2020•
edited
Loading

gpetrouDec 27, 2020•
edited
Loading

filmorDec 31, 2020•
edited
Loading