-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
gh-138004: Encode thread names on Solaris-based platforms as ASCII and fix main/worker threading test #138017
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
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Modules/_threadmodule.c
Outdated
| name_encoded = PyUnicode_AsEncodedString(name_obj, "ascii", "replace"); | ||
| if (name_encoded == NULL) { | ||
| return NULL; | ||
| } | ||
| #ifdef _PYTHREAD_NAME_MAXLEN | ||
| if (PyBytes_GET_SIZE(name_encoded) > _PYTHREAD_NAME_MAXLEN) { | ||
| PyObject *truncated; | ||
| truncated = PyBytes_FromStringAndSize(PyBytes_AS_STRING(name_encoded), | ||
| _PYTHREAD_NAME_MAXLEN); | ||
| if (truncated == NULL) { | ||
| Py_DECREF(name_encoded); | ||
| return NULL; | ||
| } | ||
| Py_SETREF(name_encoded, truncated); | ||
| } | ||
| #endif | ||
| name = PyBytes_AS_STRING(name_encoded); | ||
| #ifdef __APPLE__ | ||
| rc = pthread_setname_np(name); | ||
| #elif defined(__NetBSD__) | ||
| thread = pthread_self(); | ||
| rc = pthread_setname_np(thread, "%s", (void *)name); | ||
| #elif defined(HAVE_PTHREAD_SETNAME_NP) | ||
| thread = pthread_self(); | ||
| rc = pthread_setname_np(thread, name); | ||
| #else /* defined(HAVE_PTHREAD_SET_NAME_NP) */ | ||
| thread = pthread_self(); | ||
| rc = 0; | ||
| pthread_set_name_np(thread, name); | ||
| #endif | ||
| Py_DECREF(name_encoded); |
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.
Do not duplicate such complex code. Move it to function and reuse. Or use a loop (but this may be more complicated).
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 moved to a function and reused. Despite most tests passing, I am receiving the following for the "Check if generated files are up to date" test:
/usr/bin/ld: Modules/_threadmodule.o: in function `_thread_set_name_impl':
/home/runner/work/cpython/cpython/./Modules/_threadmodule.c:2617:(.text+0x2ed5): undefined reference to `_set_thread_name'
/usr/bin/ld: /home/runner/work/cpython/cpython/./Modules/_threadmodule.c:2637:(.text+0x2f99): undefined reference to `_set_thread_name'
collect2: error: ld returned 1 exit status
make: *** [Makefile:1927: Programs/_freeze_module] Error 1
Error: Process completed with exit code 2.
Please let me know if you see the issue. It appeared to be working before changing to function.
Lib/test/test_threading.py
Outdated
| thread.join() | ||
| self.assertEqual(work_name, expected, | ||
| f"{len(work_name)=} and {len(expected)=}") | ||
| except OSError as exc: |
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.
Is OSError even raised here? The test failure was different -- that work_name was unexpectedly empty.
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.
Added:
if any(ord(c) > 127 for c in name) and (not work_name or work_name == ""):
self.skipTest(f"Platform does not support non-ASCII thread names: got empty name for {name!r}")
self.assertEqual(work_name, expected,
f"{len(work_name)=} and {len(expected)=}")
Kept OSError fallback because it was shown in original issue, but can remove if needed.
>>> import _thread
>>> _thread.set_name('€')
Traceback (most recent call last):
File "<python-input-1>", line 1, in <module>
_thread.set_name('€')
~~~~~~~~~~~~~~~~^^^^^
OSError: [Errno 22] Invalid argument
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.
It was shown that _thread.set_name() fails, but that OSError is not leaked from the Thread code.
Misc/NEWS.d/next/Core_and_Builtins/2025-08-21-06-31-42.gh-issue-138004.FH2Hre.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Core_and_Builtins/2025-08-21-06-31-42.gh-issue-138004.FH2Hre.rst
Outdated
Show resolved
Hide resolved
|
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 |
|
Please see comments regarding the requested changes. OS tests passed, but a function not found error occurred in "Tests / Check if generated files are up to date (pull_request)", which I am having trouble fixing. Apologies - this is my first attempt at contributing to Python. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @serhiy-storchaka: please review the changes made to this pull request. |
Modules/_threadmodule.c
Outdated
| #ifdef _PYTHREAD_NAME_MAXLEN | ||
| if (PyBytes_GET_SIZE(name_encoded) > _PYTHREAD_NAME_MAXLEN) { | ||
| PyObject *truncated = PyBytes_FromStringAndSize(PyBytes_AS_STRING(name_encoded), _PYTHREAD_NAME_MAXLEN); | ||
| if (truncated == NULL) { | ||
| Py_DECREF(name_encoded); | ||
| return NULL; | ||
| } | ||
| Py_SETREF(name_encoded, truncated); | ||
| } | ||
| #endif |
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.
Again, please try to avoid duplicating code. This should be factored out into its own function. Here's an outline:
static PyObject *
get_truncated(PyObject *name_encoded /* stolen */)
{
#ifdef _PYTHREAD_NAME_MAXLEN
if (PyBytes_GET_SIZE(name_encoded) > _PYTHREAD_NAME_MAXLEN) {
PyObject *truncated = PyBytes_FromStringAndSize(PyBytes_AS_STRING(name_encoded), _PYTHREAD_NAME_MAXLEN);
if (truncated == NULL) {
Py_DECREF(name_encoded);
return NULL;
}
Py_SETREF(name_encoded, truncated);
}
#endif
return name_encoded;
}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.
Or simply include the encoding and truncating code in _set_thread_name(). BTW, there is no need to use underscored names here.
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.
Created function encode_thread_name and used to replace duplicated code
Lib/test/test_threading.py
Outdated
|
|
||
| with warnings.catch_warnings(record=True) as warnings_log: | ||
| CustomRLock(1, b=2) | ||
|
|
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.
Stray newline change:
Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
|
Thank you. I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @serhiy-storchaka: please review the changes made to this pull request. |
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.
LGTM
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.
LGTM. 👍
|
Thanks @jadonduff for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…38017) Encode Solaris/Illumos thread names to ASCII, since OpenIndiana does not support non-ASCII names. Add tests for setting non-ASCII name for the main thread. (cherry picked from commit c19db1d) Co-authored-by: jadonduff <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>
|
GH-138384 is a backport of this pull request to the 3.14 branch. |
|
Thanks @jadonduff for your fix. |
…38017) Encode Solaris/Illumos thread names to ASCII, since OpenIndiana does not support non-ASCII names. Add tests for setting non-ASCII name for the main thread. Co-authored-by: Serhiy Storchaka <[email protected]>
#138384) gh-138004: Fix setting a thread name on OpenIndiana (GH-138017) Encode Solaris/Illumos thread names to ASCII, since OpenIndiana does not support non-ASCII names. Add tests for setting non-ASCII name for the main thread. (cherry picked from commit c19db1d) Co-authored-by: jadonduff <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>
Fallback to ASCII in _thread.set_name() when pthread_setname_np() rejects UTF-8
Summary
Issue:
test_set_namefails on OpenIndiana (and Solaris descendants).It seems that
pthread_setname_np()only accepts ASCII-only names there:Fix
1.
_threadmodule.cpthread_setname_np()as usual.EINVAL, fall back to ASCII encoding using"replace"(non-ASCII →?).Explanation:
OSError(22)for non-ASCII names.2.
test_thread.pytest_set_nameto be lenient on platforms that reject non-ASCII names.OSError(22)occurs and the attempted name contained non-ASCII, the test is skipped instead of failing.Explanation:
Verification