Skip to content

gh-138004: fix threadmodule ascii and make thread naming test more lenient #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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Lib/test/test_threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -2241,6 +2241,7 @@ def __init__(self, a, *, b) -> None:

with warnings.catch_warnings(record=True) as warnings_log:
CustomRLock(1, b=2)

Copy link
Member

Choose a reason for hiding this comment

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

Stray newline change:

Suggested change

self.assertEqual(warnings_log, [])

class EventTests(lock_tests.EventTests):
Expand Down Expand Up @@ -2360,6 +2361,9 @@ def work():
thread = threading.Thread(target=work, name=name)
thread.start()
thread.join()
# If the name is non-ASCII and the result is empty, skip (platform limitation)
Copy link
Member

Choose a reason for hiding this comment

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

Do not repeat the code in a comment.

if not name.isascii() and not 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)=}")

Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ Weilin Du
John DuBois
Paul Dubois
Jacques Ducasse
Jadon Duff
Andrei Dorian Duma
Graham Dumpleton
Quinn Dunkan
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
_thread.set_name() now retries with an ASCII fallback if pthread_setname_np() rejects UTF-8 names on some POSIX-compliant platforms.
122 changes: 75 additions & 47 deletions Modules/_threadmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
#ifdef HAVE_SIGNAL_H
# include <signal.h> // SIGINT
#endif
#ifdef HAVE_PTHREAD_H
# include <pthread.h>
#endif
#include <errno.h>

// ThreadError is just an alias to PyExc_RuntimeError
#define ThreadError PyExc_RuntimeError
Expand Down Expand Up @@ -71,6 +75,64 @@ get_thread_state_by_cls(PyTypeObject *cls)
return get_thread_state(module);
}

// Helper to set the thread name using platform-specific APIs
static int
set_native_thread_name(const char *name)
{
int rc;
#ifdef __APPLE__
rc = pthread_setname_np(name);
#elif defined(__NetBSD__)
pthread_t thread = pthread_self();
rc = pthread_setname_np(thread, "%s", (void *)name);
#elif defined(HAVE_PTHREAD_SETNAME_NP)
pthread_t thread = pthread_self();
rc = pthread_setname_np(thread, name);
#elif defined(HAVE_PTHREAD_SET_NAME_NP)
pthread_t thread = pthread_self();
pthread_set_name_np(thread, name);
rc = 0; /* pthread_set_name_np() returns void */
#endif
return rc;
}

// Helper to encode and truncate thread name
static PyObject *
encode_thread_name(PyObject *name_obj, const char *encoding)
{
#ifdef __sun
// Solaris always uses UTF-8
encoding = "utf-8";
#endif
PyObject *name_encoded = PyUnicode_AsEncodedString(name_obj, encoding, "replace");
if (name_encoded == NULL) {
return NULL;
}
#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);
Py_DECREF(name_encoded);
return truncated;
Py_DECREF(name_encoded);
return truncated;
Comment on lines +116 to +117
Copy link
Member

Choose a reason for hiding this comment

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

Dead code.

}
#endif
return name_encoded;
}

// Helper to encode, set, and cleanup thread name in one step
static int
set_thread_name_with_encoding(PyObject *name_obj, const char *encoding)
{
PyObject *name_encoded = encode_thread_name(name_obj, encoding);
if (name_encoded == NULL) {
return -1; // error, exception set
Copy link
Member

Choose a reason for hiding this comment

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

What happens at the caller place when set_thread_name_with_encoding() returns -1?

}
const char *name = PyBytes_AS_STRING(name_encoded);
int rc = set_native_thread_name(name);
Py_DECREF(name_encoded);
return rc;
Comment on lines +127 to +134
Copy link
Member

Choose a reason for hiding this comment

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

You can now inline set_native_thread_name() and encode_thread_name() as it was in the original code.

}

#ifdef MS_WINDOWS
typedef HRESULT (WINAPI *PF_GET_THREAD_DESCRIPTION)(HANDLE, PCWSTR*);
Expand Down Expand Up @@ -2581,65 +2643,32 @@ _thread_set_name_impl(PyObject *module, PyObject *name_obj)
/*[clinic end generated code: output=402b0c68e0c0daed input=7e7acd98261be82f]*/
{
#ifndef MS_WINDOWS
#ifdef __sun
// Solaris always uses UTF-8
const char *encoding = "utf-8";
#else
// Encode the thread name to the filesystem encoding using the "replace"
// error handler
Comment on lines -2584 to -2589
Copy link
Member

Choose a reason for hiding this comment

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

Why remove all this?

Copy link
Author

Choose a reason for hiding this comment

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

moved to encode_thread_name

PyInterpreterState *interp = _PyInterpreterState_GET();
const char *encoding = interp->unicode.fs_codec.encoding;
#endif
PyObject *name_encoded;
name_encoded = PyUnicode_AsEncodedString(name_obj, encoding, "replace");
if (name_encoded == NULL) {
return NULL;
}

#ifdef _PYTHREAD_NAME_MAXLEN
// Truncate to _PYTHREAD_NAME_MAXLEN bytes + the NUL byte if needed
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

const char *name = PyBytes_AS_STRING(name_encoded);
#ifdef __APPLE__
int rc = pthread_setname_np(name);
#elif defined(__NetBSD__)
pthread_t thread = pthread_self();
int rc = pthread_setname_np(thread, "%s", (void *)name);
#elif defined(HAVE_PTHREAD_SETNAME_NP)
pthread_t thread = pthread_self();
int rc = pthread_setname_np(thread, name);
#else /* defined(HAVE_PTHREAD_SET_NAME_NP) */
pthread_t thread = pthread_self();
int rc = 0; /* pthread_set_name_np() returns void */
pthread_set_name_np(thread, name);
#endif
Py_DECREF(name_encoded);
int rc = set_thread_name_with_encoding(name_obj, encoding);
if (rc) {
errno = rc;
int err = rc;
Copy link
Member

Choose a reason for hiding this comment

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

Why it was needed to introduce the err variable?

if (err == EINVAL && strcmp(encoding, "ascii") != 0) {
rc = set_thread_name_with_encoding(name_obj, "ascii");
if (rc) {
errno = rc;
return PyErr_SetFromErrno(PyExc_OSError);
}
Py_RETURN_NONE;
}
errno = err;
return PyErr_SetFromErrno(PyExc_OSError);
}
Py_RETURN_NONE;
#else
// Windows implementation
assert(pSetThreadDescription != NULL);

Copy link
Member

Choose a reason for hiding this comment

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

Those blank lines could be kept as this part of the code is not touched.

Py_ssize_t len;
wchar_t *name = PyUnicode_AsWideCharString(name_obj, &len);
if (name == NULL) {
return NULL;
}

if (len > _PYTHREAD_NAME_MAXLEN) {
// Truncate the name
Py_UCS4 ch = name[_PYTHREAD_NAME_MAXLEN-1];
Expand All @@ -2650,7 +2679,6 @@ _thread_set_name_impl(PyObject *module, PyObject *name_obj)
name[_PYTHREAD_NAME_MAXLEN] = 0;
}
}

HRESULT hr = pSetThreadDescription(GetCurrentThread(), name);
PyMem_Free(name);
if (FAILED(hr)) {
Expand Down Expand Up @@ -2894,4 +2922,4 @@ PyMODINIT_FUNC
PyInit__thread(void)
{
return PyModuleDef_Init(&thread_module);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change (newline?).

Copy link
Author

Choose a reason for hiding this comment

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

May have accidentally added a newline in a previous commit. It now matches the current _threadmodule.c in the main branch.

Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change.

Loading