-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-59705: Add _thread.set_name() function #127338
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
Conversation
On Linux, threading.Thread now sets the thread name to the operating system. configure now checks if pthread_setname_np() function is available.
This implementation is very basic on purpose. I plan to add support for more platform in follow-up PRs.
Demo 1 (main thread): $ ./python
>>> import os
>>> pid=os.getpid()
>>> with open(f"/proc/{pid}/task/{pid}/comm") as fp: print(f"comm = {fp.read()!r}")
...
comm = 'python\n'
>>> import _thread; _thread.set_name("demo")
>>> with open(f"/proc/{pid}/task/{pid}/comm") as fp: print(f"comm = {fp.read()!r}")
...
comm = 'demo\n' Demo 2 (thread): $ ./python
>>> import threading, os, time
>>> os.getpid()
81921
>>> t=threading.Thread(target=time.sleep, args=(60,), name="sleeper")
>>> t.start()
^Z
$ cat /proc/81921/task/81927/comm
sleeper See also a previous attempt to implement the feature: #14578 |
I changed my mind and added a private |
@pitrou @encukou @serhiy-storchaka: Would you mind to review this change? It's to set the thread name in threading.Thread to the operating system. |
I modified Truncating the string in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you!
The truncation is not pretty with non-ASCII names. I guess codepoint-preserving truncation is not worth the effort, and Linux tools need to deal with thread names being arbitrary bytes.
But, we can test the edge cases, to ensure this quality-of-life enhancement doesn't start raising exceptions in working code.
b9f2359
to
78a9ab9
Compare
@encukou: I addressed your reviews. Please review the updated PR. I added tests on long names and non-ASCII names. |
Refactor also tests.
@encukou: Maybe the |
You can use FS_NONASCII. You can also use TESTFN_UNDECODABLE to test that it works with arbitrary bytes and TESTFN_UNENCODABLE to test for encoding error. Is it a hard limit for the size? Is it the same on other platforms? I would prefer to use a named constant instead of magic numbers 15, 16, 17. |
Yes. Using a longer name fails with ERANGE.
It's 16 bytes on Linux and 64 bytes on macOS, so no, it's not the same.
I failed to find a public constant for these limits. For example, Darwin MAXTHREADNAMESIZE constant is private (I'm not 100% sure, but I don't have macOS so I cannot check manually, I only read the code). |
Ok, I added tests using FS_NONASCII and TESTFN_UNENCODABLE. |
Well, then hardcoding limits for known platforms is okay. We could also determine it at runtime, but this would be too complicated.
It is the content of the file, not its name. There is a flaw in this example: what encoding do you use to decode it? How do you read the name of the thread in other process with different locale? |
@serhiy-storchaka: I addressed your review, please review the updated PR.
open() uses the current LC_CTYPE locale encoding by default.
You have the same problem with file content. It's not a new problem. IMO the Python filesystem encoding is a better choice than UTF-8 for the thread name. |
It is only a large problem if you use locale encoding for file content. If you use a fixed encoding (e.g. UTF-8) or write encoding as a metadata before writing thee encoded content, it is not a problem or a lesser problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with some decisions, but do not want to block the PR. We can fix this later.
Co-authored-by: Serhiy Storchaka <[email protected]>
errno = rc; | ||
return PyErr_SetFromErrno(PyExc_OSError); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pthread_getname_np
should add a trailing NUL byte, but like everything here, that's platform-specific. I suggest being defensive here.
name[Py_ARRAY_LENGTH(name)-1] = 0; | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On what platform it does not add the null byte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null byte is added on all supported platforms. Before I made sure that the buffer always ended with a null byte, but @serhiy-storchaka asked me to remove it. Let's be optimistic. We can adjust the code later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, ship it :)
Or make a few more changes if you want it to be more perfect.
size_t len = PyBytes_GET_SIZE(name_encoded); | ||
if (len > PYTHREAD_NAME_MAXLEN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also inline len
. It is only used here.
On Linux, threading.Thread now sets the thread name to the operating system. * configure now checks if pthread_getname_np() and pthread_setname_np() functions are available. * Add PYTHREAD_NAME_MAXLEN macro. * Add _thread._NAME_MAXLEN constant for test_threading. Co-authored-by: Serhiy Storchaka <[email protected]>
I am sorry I didn't get to this earlier; on Solaris, the tests are green and all works as expected. There is one small issue with Solaris detection in |
On Linux, threading.Thread now sets the thread name to the operating system.
configure now checks if pthread_setname_np() function is available.