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

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

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 9, 2025

Implement set_name() with SetThreadDescription() and _get_name() with GetThreadDescription(). If SetThreadDescription() or GetThreadDescription() is not available in kernelbase.dll, delete the method when the _thread module is imported.

Truncate the thread name to 32766 characters.

set_name() raises ValueError if the name contains an embedded null character.

Implement set_name() with SetThreadDescription() and _get_name() with
GetThreadDescription(). If SetThreadDescription() or
GetThreadDescription() is not available in kernelbase.dll, delete the
method when the _thread module is imported.

Truncate the thread name to an arbitrary limit of 64 characters.

set_name() raises ValueError if the name contains an embedded null
character.

Co-authored-by: Eryk Sun <[email protected]>
@vstinner
Copy link
Member Author

vstinner commented Jan 9, 2025

Code based on @eryksun's code: #59705 (comment). Differences with his code:

  • I only look for functions in kernelbase.dll, not in api-ms-win-core-processthreads-l1-1-3.dll (kernel32).
  • set_name() doesn't ignore E_NOTIMPL. That's fine since the threading module ignores any OSError. If Python is run on Windows older than 10.0.14393 (1607), set_name() failure would just be ignored silently.
  • I used the PyUnicode API to convert to/from wchar_t*.

Truncate the thread name to an arbitrary limit of 64 characters.

I copied this limit from @eryksun's code. Maybe it can be extended to 32766 characters?

set_name() raises ValueError if the name contains an embedded null character.

If it's a blocker issue, I can write the code differently to truncate to the first null character instead.

@vstinner
Copy link
Member Author

vstinner commented Jan 9, 2025

cc @serhiy-storchaka

@vstinner
Copy link
Member Author

vstinner commented Jan 9, 2025

set_name() raises ValueError if the name contains an embedded null character.

I misunderstood the code, in fact, the name is truncated at the first null character (ValueError is not raised): same behavior than Linux/macOS/FreeBSD/etc.

@vstinner
Copy link
Member Author

cc @zooba @encukou

@vstinner
Copy link
Member Author

The alternative to SetThreadDescription() (Windows 10 and newer) is to call RaiseException(0x406D1388, ...). Example: msys2-contrib/mingw-w64@0d95c79. From what I understsood, RaiseException(...) is only understood by debuggers.

The Process Explorer tool doesn't show thread names.

@encukou
Copy link
Member

encukou commented Jan 14, 2025

I'm not familiar with Windows API, but this looks right to me. A review from @python/windows-team would be great though :)

(Specifically: this is a best-effort attempt to add info to aid debugging or understanding what's going on in a system: we want to as much as we reasonably can to the OS, but it's OK drop part of the name or not set it at all, and errors should be discarded since the user didn't really ask for this. And _get_name is private (test-only); this is not a reliable API to get back a Python string.)

@vstinner
Copy link
Member Author

I plan to merge this change at the beginning of next week (January 20th).

@vstinner
Copy link
Member Author

I fixed the truncation for surrogate pairs and I added tests with non-BMP characters (creating surrogate pairs on Windows).

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I am not a Windows expert, but the code LGTM. Although I am not sure that we should set such small artificial limit.

There are also some issues in tests.

Lib/test/test_threading.py Show resolved Hide resolved
Lib/test/test_threading.py Outdated Show resolved Hide resolved
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@vstinner
Copy link
Member Author

Although I am not sure that we should set such small artificial limit.

Ok, I increased the limit from 64 to 32766 characters.

@vstinner vstinner enabled auto-merge (squash) January 17, 2025 12:20
@vstinner
Copy link
Member Author

Thanks for the reviews. I enabled auto-merge.

@@ -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.

PC/pyconfig.h.in Outdated
@@ -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.

@zooba zooba disabled auto-merge January 17, 2025 12:23
Remove the module methods if GetModuleHandleW() fails.
@vstinner
Copy link
Member Author

@zooba: Please review the updated PR. I fixed thread_module_exec() to remove the module methods if GetModuleHandleW() fails.

@zooba
Copy link
Member

zooba commented Jan 17, 2025

LGTM

@vstinner vstinner merged commit d7f703d into python:main Jan 17, 2025
38 of 39 checks passed
@vstinner vstinner deleted the win_thread_descr branch January 17, 2025 13:55
@vstinner
Copy link
Member Author

Merged. Thanks for reviews.

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

Successfully merging this pull request may close these issues.

4 participants