Skip to content
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

bpo-36084: add native thread ID (TID) to threading.Thread objects (V2) #13463

Merged
merged 28 commits into from May 22, 2019

Conversation

Copy link
Contributor

@jaketesler jaketesler commented May 21, 2019

This is the second version of PR #11993 (original reverted due to test failures on unsupported operating systems).

cc @vstinner @pitrou

From the original PR:

This functionality adds a native Thread ID to threading.Thread objects. This ID (TID), similar to the PID of a process, is assigned by the OS (kernel) and is generally used for externally monitoring resources consumed by the running thread (or process).
This does not replace the ident attribute within Thread objects, which is assigned by the Python interpreter and is guaranteed as unique for the lifetime of the Python instance.

This new functionality fully supports Linux, Apple, and Windows platforms.

https://bugs.python.org/issue36084

jaketesler and others added 22 commits Feb 22, 2019
…jects now have a tid property (kernel thread ID)
…c operation, since the functionality now supports Windows as well; cleaned up and clarified doc notes, updated version added number (3.8)
… ID functions depending on system type and functional availability; Threading module now returns `None` if the thread has not been started, or if the Native ID functionality does not exist for the current operating system.
Lib/_dummy_thread.py Outdated Show resolved Hide resolved
Lib/test/test_threading.py Outdated Show resolved Hide resolved
Lib/threading.py Outdated Show resolved Hide resolved
Lib/threading.py Outdated Show resolved Hide resolved
Python/thread_pthread.h Outdated Show resolved Hide resolved
Python/thread_pthread.h Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 21, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

@jaketesler
Copy link
Author

@jaketesler jaketesler commented May 21, 2019

Per @bedevere-bot
I have made the requested changes; please review again.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 21, 2019

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

Python/thread_nt.h Outdated Show resolved Hide resolved
Doc/library/_thread.rst Outdated Show resolved Hide resolved
Doc/library/threading.rst Outdated Show resolved Hide resolved
Doc/library/threading.rst Outdated Show resolved Hide resolved
Doc/library/threading.rst Outdated Show resolved Hide resolved
Python/thread_pthread.h Show resolved Hide resolved
Python/thread_pthread.h Show resolved Hide resolved
Python/thread_pthread.h Outdated Show resolved Hide resolved
Python/thread_pthread.h Outdated Show resolved Hide resolved
Python/thread_pthread.h Show resolved Hide resolved
@jaketesler
Copy link
Author

@jaketesler jaketesler commented May 22, 2019

@vstinner please advise if there are any additional suggestions you have or changes that should be implemented before the merge.

@aixtools
Copy link

@aixtools aixtools commented May 22, 2019

This is the second version of PR #11993 (original reverted due to test failures on unsupported operating systems).

cc @vstinner @pitrou

From the original PR:

This functionality adds a native Thread ID to threading.Thread objects. This ID (TID), similar to the PID of a process, is assigned by the OS (kernel) and is generally used for externally monitoring resources consumed by the running thread (or process).
This does not replace the ident attribute within Thread objects, which is assigned by the Python interpreter and is guaranteed as unique for the lifetime of the Python instance.
This new functionality fully supports Linux, Apple, and Windows platforms.

I expect the reason it broke AIX - is because AIX supports - for nearly 30 years is my guess (not known if it was already in AIX 3.X, but always in AIX 4.X).
From the AIX 6.1 documentation:

pthread_self Subroutine
Purpose
Returns the calling thread's ID.
Library
Threads Library (libpthreads.a)
Syntax
#include <pthread.h>
pthread_t pthread_self (void);
Description
The pthread_self subroutine returns the calling thread's ID.
Note: The pthread.h header file must be the first included file of each source file using the threads
library. Otherwise, the -D_THREAD_SAFE compilation flag should be used, or the cc_r compiler
used. In this case, the flag is automatically set.
Return Values
The calling thread's ID is returned.
Errors
No errors are defined.
The pthread_self function will not return an error code of EINTR.

The 'key' issue being that <pthread.h> needs to be first (or -D_THREAD_SAFE, which may be 'better' to ensure that all C-code is compiled "thread-safe" when using gcc. I normally use xlc_r, which sets _THREAD_SAFE - but I cannot assume that gcc (which is what the AIX bots currently use) are compiling everything "thread-safe".
Iirc, among other things, "thread-safe" means the linker links with the "thread_safe" libraries (provided by IBM. No guarantee that third-party libraries are thread-safe. etc..

Anyway, moving forward - how can I best contribute some code specific to AIX. Never tried working "together" before.

@jaketesler
Copy link
Author

@jaketesler jaketesler commented May 22, 2019

To be clear, in all the POSIX-based platforms that I’ve worked with to build this feature (notably excluding AIX, which I have zero experience with), *_self()-type functions are NOT the same value/ID as the one used by this feature.

Can you confirm whether or not AIX has the capability to return the integral ID, or whether the pthread_self() function simply returns the same struct reference as other POSIX systems? Given the return type pthread_t, at first glance I’m inclined to believe it’s not the same functionality.

@jaketesler
Copy link
Author

@jaketesler jaketesler commented May 22, 2019

Looking at docs, I think this might be what we’re looking for: https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.basetrf2/thread_self.htm

I will look into this and see if it’d be easy to add for this PR.

@pitrou
Copy link

@pitrou pitrou commented May 22, 2019

I'd recommend tackling AIX as a separate PR.

@jaketesler
Copy link
Author

@jaketesler jaketesler commented May 22, 2019

Either way :)

In that case, I think this is mostly good to go (pending review of course).

@vstinner
Copy link

@vstinner vstinner commented May 22, 2019

I could continue to nitpick to death, but well, you addressed my main concern: not add the function/attribute on unsupported platforms, so I approve your change. Thanks for fixing it!

@vstinner vstinner merged commit b121f63 into python:master May 22, 2019
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants