10000 gh-59705: Implement _thread.set_name() on Windows by vstinner · Pull Request #128675 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-59705: Implement _thread.set_name() on Windows #128675

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 17, 2025
Merged

gh-59705: Implement _thread.set_name() on Windows #128675

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
479c7fb
gh-59705: Implement _thread.set_name() on Windows
vstinner Jan 9, 2025
d588103
Embedded null characters are accepted
vstinner Jan 9, 2025
8f2ead7
Fix trailing surrogate characters
vstinner Jan 16, 2025
f1c5232
Add more non-BMP tests; fix test on Windows
vstinner Jan 17, 2025
ba09ba1
Set limit to 32766
vstinner Jan 17, 2025
8148a96
Fix set_name() error handling
vstinner Jan 17, 2025
0ff0567
Fix thread_module_exec()
vstinner Jan 17, 2025
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
30 changes: 22 additions & 8 deletions Lib/test/test_threading.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2130,6 +2130,11 @@ def test_set_name(self):

# Test long non-ASCII name (truncated)
"x" * (limit - 1) + "é€",

# Test long non-BMP names (truncated) creating surrogate pairs
# on Windows
"x" * (limit - 1) + "\U0010FFFF",
"x" * (limit - 2) + "\U0010FFFF" * 2,
]
if os_helper.FS_NONASCII:
tests.append(f"nonascii:{os_helper.FS_NONASCII}")
Expand All @@ -2146,15 +2151,24 @@ def work():
work_name = _thread._get_name()

for name in tests:
encoded = name.encode(encoding, "replace")
if b'\0' in encoded:
encoded = encoded.split(b'\0', 1)[0]
if truncate is not None:
encoded = encoded[:truncate]
if sys.platform.startswith("solaris"):
expected = encoded.decode("utf-8", "surrogateescape")
if not support.MS_WINDOWS:
encoded = name.encode(encoding, "replace")
if b'\0' in encoded:
encoded = encoded.split(b'\0', 1)[0]
if truncate is not None:
encoded = encoded[:truncate]
if sys.platform.startswith("solaris"):
expected = encoded.decode("utf-8", "surrogateescape")
else:
expected = os.fsdecode(encoded)
else:
expected = os.fsdecode(encoded)
expected = name[:truncate]
if ord(expected[-1]) > 0xFFFF:
# truncate the last non-BMP character to omit a lone
# surrogate character
expected = expected[:-1]
if '\0' in expected:
expected = expected.split('\0', 1)[0]

with self.subTest(name=name, expected=expected):
work_name = None
Expand Down
83 changes: 81 additions & 2 deletions Modules/_threadmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ get_thread_state(PyObject *module)
}


#ifdef MS_WINDOWS
typedef HRESULT (WINAPI *PF_GET_THREAD_DESCRIPTION)(HANDLE, PCWSTR*);
typedef HRESULT (WINAPI *PF_SET_THREAD_DESCRIPTION)(HANDLE, PCWSTR);
static PF_GET_THREAD_DESCRIPTION pGetThreadDescription = NULL;
static PF_SET_THREAD_DESCRIPTION pSetThreadDescription = NULL;
#endif


/*[clinic input]
module _thread
[clinic start generated code]*/
Expand Down Expand Up @@ -2364,7 +2372,7 @@ Internal only. Return a non-zero integer that uniquely identifies the main threa
of the main interpreter.");


#ifdef HAVE_PTHREAD_GETNAME_NP
#if defined(HAVE_PTHREAD_GETNAME_NP) || defined(MS_WINDOWS)
/*[clinic input]
_thread._get_name

Expand All @@ -2375,6 +2383,7 @@ static PyObject *
_thread__get_name_impl(PyObject *module)
/*[clinic end generated code: output=20026e7ee3da3dd7 input=35cec676833d04c8]*/
{
#ifndef MS_WINDOWS
// Linux and macOS are limited to respectively 16 and 64 bytes
char name[100];
pthread_t thread = pthread_self();
Expand All @@ -2389,11 +2398,26 @@ _thread__get_name_impl(PyObject *module)
#else
return PyUnicode_DecodeFSDefault(name);
#endif
#else
// Windows implementation
assert(pGetThreadDescription != NULL);

wchar_t *name;
HRESULT hr = pGetThreadDescription(GetCurrentThread(), &name);
if (FAILED(hr)) {
PyErr_SetFromWindowsErr(0);
return NULL;
}

PyObject *name_obj = PyUnicode_FromWideChar(name, -1);
LocalFree(name);
return name_obj;
#endif
}
#endif // HAVE_PTHREAD_GETNAME_NP


#ifdef HAVE_PTHREAD_SETNAME_NP
#if defined(HAVE_PTHREAD_SETNAME_NP) || defined(MS_WINDOWS)
/*[clinic input]
_thread.set_name

Expand All @@ -2406,6 +2430,7 @@ static PyObject *
_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";
Expand Down Expand Up @@ -2451,6 +2476,35 @@ _thread_set_name_impl(PyObject *module, PyObject *name_obj)
return PyErr_SetFromErrno(PyExc_OSError);
}
Py_RETURN_NONE;
#else
// Windows implementation
assert(pSetThreadDescription != NULL);

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];
if (Py_UNICODE_IS_HIGH_SURROGATE(ch)) {
name[PYTHREAD_NAME_MAXLEN-1] = 0;
}
else {
name[PYTHREAD_NAME_MAXLEN] = 0;
}
}

HRESULT hr = pSetThreadDescription(GetCurrentThread(), name);
PyMem_Free(name);
if (FAILED(hr)) {
PyErr_SetFromWindowsErr(0);
return NULL;
}
Py_RETURN_NONE;
#endif
}
#endif // HAVE_PTHREAD_SETNAME_NP

Expand Down Expand Up @@ -2594,6 +2648,31 @@ thread_module_exec(PyObject *module)
}
#endif

#ifdef MS_WINDOWS
HMODULE kernelbase = GetModuleHandleW(L"kernelbase.dll");
if (kernelbase != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

If this check fails, we likely want to report a system error since everything is broken, but at least we should call DelAttr to remove the functions, since the function pointers will be null.

Copy link
Member Author

Choose a reason for hiding this comment

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

set_name()/_get_name() is a minor feature of the _thread module, I would prefer to not prevent to import _thread if loading kernelbase.dll fails for whatever reason.

but at least we should call DelAttr to remove the functions, since the function pointers will be null.

Oops, my code was wrong. Fixed.

if (pGetThreadDescription == NULL) {
pGetThreadDescription = (PF_GET_THREAD_DESCRIPTION)GetProcAddress(
kernelbase, "GetThreadDescription");
}
if (pSetThreadDescription == NULL) {
pSetThreadDescription = (PF_SET_THREAD_DESCRIPTION)GetProcAddress(
kernelbase, "SetThreadDescription");
}

if (pGetThreadDescription == NULL) {
if (PyObject_DelAttrString(module, "_get_name") < 0) {
return -1;
}
}
if (pSetThreadDescription == NULL) {
if (PyObject_DelAttrString(module, "set_name") < 0) {
return -1;
}
}
}
#endif

return 0;
}

Expand Down
10 changes: 5 additions & 5 deletions Modules/clinic/_threadmodule.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions PC/pyconfig.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,4 +753,8 @@ Py_NO_ENABLE_SHARED to find out. Also support MS_NO_COREDLL for b/w compat */
/* Define if libssl has X509_VERIFY_PARAM_set1_host and related function */
#define HAVE_X509_VERIFY_PARAM_SET1_HOST 1

// Truncate the thread name to 64 characters. The OS limit is 32766 wide
// characters, but long names aren't of practical use.
#define PYTHREAD_NAME_MAXLEN 64
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason for us to artificially limit this at all, but I'd rather be much closer to the real limit than this. "Aren't of practical use" is a value judgement, not a technical limit.

Also, there's no C API for this, so we probably don't need a public C constant for the limit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the limit to 32766 characters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, there's no C API for this, so we probably don't need a public C constant for the limit.

I can rename the macro use _Py prefix. But that unrelated to the Windows implementation, so I would prefer to do it in a separated change. Non-Windows platforms use the same macro.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, there's no C API for this, so we probably don't need a public C constant for the limit.

I created #128945 to make the macro private.


#endif /* !Py_CONFIG_H */
Loading
0