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 #11993

Merged
merged 20 commits into from
May 12, 2019

Conversation

jaketesler
Copy link
Contributor

@jaketesler jaketesler commented Feb 22, 2019

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

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

…c operation, since the functionality now supports Windows as well; cleaned up and clarified doc notes, updated version added number (3.8)
Copy link
Contributor

@datalogics-robb datalogics-robb left a comment

Choose a reason for hiding this comment

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

On POSIX platforms, pthread_create returns the thread ID, which seems more robust, clean, and it should work on ALL POSIX platforms, not just mac and Linux.

Copy link
Contributor

@skoslowski skoslowski left a comment

Choose a reason for hiding this comment

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

Tried this on my machine (F29) - works great.
May I suggest to add some tests, e.g. skoslowski@795643a

@jaketesler
Copy link
Contributor Author

On POSIX platforms, pthread_create returns the thread ID, which seems more robust, clean, and it should work on ALL POSIX platforms, not just mac and Linux.

@datalogics-robb do you think this change is necessary? Looking further into the C side of things, the mechanism for creating threads relies upon returning (what becomes) the thread's ident, and I'm not sure how to capture + store the return value of pthread_create() into the thread at that stage in the process. The way it's designed right now was intended to mimic how ident is created and stored as a Python variable into the thread object.

Tried this on my machine (F29) - works great.
May I suggest to add some tests, e.g. skoslowski@795643a

@skoslowski Great idea! – I'll add those tests and commit shortly.

@vstinner
Copy link
Member

On POSIX platforms, pthread_create returns the thread ID, which seems more robust, clean, and it should work on ALL POSIX platforms, not just mac and Linux.

If you are talking about the first parameter of pthread_create(): it is returned by PyThread_start_new_thread() which is then returned by _thread.start_new_thread(). threading.Thread ignores _thread.start_new_thread() result.

See also https://www.python.org/dev/peps/pep-0539/

@jaketesler
Copy link
Contributor Author

@vstinner The value returned from _thread.start_new_thread() is the thread ident parameter. It's ignored in the call within the Threading module but is stored at a different point in the execution, I suppose.

Also, see here for why the ident and TID mechanisms have to be different. The result returned by start_new_thread() is the ident or pthread handle, versus the integral identifier (TID) returned through pthread_threadid_np() or the syscall.

@jaketesler
Copy link
Contributor Author

Also per @vstinner – 

I understand that it's not available on FreeBSD...

I am now looking into FreeBSD support, as I understand the mechanism is different than on Linux/POSIX machines. I believe it should not be too difficult to implement. (famous last words? haha)

Assuming this PR will support Windows, Linux, Mac, and FreeBSD, are there any other major architectures missing that should be added (archs where Python threading is supported)?

@vstinner vstinner requested a review from pitrou May 10, 2019 21:04
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Here are some comments.

Doc/library/threading.rst Outdated Show resolved Hide resolved
Doc/library/_thread.rst Show resolved Hide resolved
Lib/test/test_threading.py Outdated 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 Outdated Show resolved Hide resolved
Python/thread_pthread.h Outdated Show resolved Hide resolved
Modules/_threadmodule.c Outdated Show resolved Hide resolved
Lib/_dummy_thread.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

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.

@pitrou
Copy link
Member

pitrou commented May 11, 2019

On POSIX platforms, pthread_create returns the thread ID

Where is that? According to POSIX, "If successful, the pthread_create() function shall return zero; otherwise, an error number shall be returned to indicate the error."

@jaketesler
Copy link
Contributor Author

jaketesler commented May 11, 2019

From @pitrou

On POSIX platforms, pthread_create returns the thread ID

Where is that? According to POSIX, "If successful, the pthread_create() function shall return zero; otherwise, an error number shall be returned to indicate the error."

All –

pthread_create- create a new thread

int pthread_create(pthread_t *thread, const pthread_attr_t *attr, void *(*start_routine) (void *), void *arg);
...
See pthread_self(3) for further information on the thread ID returned
in *thread by pthread_create(). Unless real-time scheduling policies
are being employed, after a call to pthread_create(), it is
indeterminate which thread—the caller or the new thread—will next
execute.

Source: man7.org

To clarify, the ID returned via the first argument in the call to pthread_create(), which is also the same value as the Python ident attribute, is the same value as returned by a call to pthread_self().

This is not the same identifier as the one created by pthread_getthreadid_np(), et al. This value is the one identifying a particular thread to the kernel – the value is an integral, native system identifier, e.g. as reported by system monitoring utilities or accessed in debuggers.

@pitrou
Copy link
Member

pitrou commented May 11, 2019

Yes, so there are two problems with the pthread_t values:

  1. it's an opaque type, and could very well be a non-integer (for example a struct)
  2. it doesn't necessarily map to system thread IDs (which may or may not be a problem)

@jaketesler
Copy link
Contributor Author

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

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

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
@jaketesler
Copy link
Contributor Author

Should the commit history be squashed prior to merge?

@pitrou
Copy link
Member

pitrou commented May 12, 2019

@jaketesler We let Github squash automatically when merging, so you needn't do it yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants