Thread: Fixes to thread name #1279
Conversation
- _CFThreadSetName(): Take the thread ID, as Linux allows setting the name of another thread. On macOS just check that it is the same as pthread_self(). - On Linux, trim the name to 15 characters as that is the maximum that can be set. - When starting a thread, set it's name in pthreads if the name is non-nil. - On Linux, make _thread an Optional initialised to nil instead of setting it to pthread_t(), which gives it the value 0. This matches macOS behaviour. - TestThread.swift: Add some more tests.
|
@swift-ci please test |
| CF_SWIFT_EXPORT void _CFThreadSetName(const char *_Nullable name); | ||
| CF_SWIFT_EXPORT int _CFThreadGetName(char *buf, int length); | ||
| CF_SWIFT_EXPORT int _CFThreadSetName(pthread_t thread, const char *_Nonnull name); | ||
| CF_SWIFT_EXPORT int _CFThreadGetName(char *_Nonnull, int length); |
There was a problem hiding this comment.
Have we lost the 'buf' name here?
There was a problem hiding this comment.
Good spot, that is unintended. I will sort out a fix
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
| #elif DEPLOYMENT_TARGET_LINUX | ||
| pthread_setname_np(pthread_self(), name); | ||
| // pthread_setname_np will fail if name >= 16 characters | ||
| char short_name[16]; |
There was a problem hiding this comment.
This is technically taking a string which we assume to be constant, and then passing it to the call pointing to a locally allocated buffer. Do we know that the pthread_setname_np call is going to take a copy of that string, as opposed to just referring to it by pointer? The call appears to be going to a PRCTL(PR_SETNAME) in the Linux implementation; do we know that does the right thing?
I think it would be better to have an assert and return the error message if the thread name is too long, rather than trying to do the defensive copy here.
There was a problem hiding this comment.
Does const say anything about the lifetime of a pointer? What happens if the String held in Thread type is deallocated before the thread terminates?
I was being defensive because setting thread names seems to be mostly for debugging. An assert might be a bit excessive, however the copy could just be removed completely and the call allowed to fail with the ERANGE that pthread_setname_np would normally return.
There was a problem hiding this comment.
If we're changing this to return an Exxx code, I'd say let the caller deal with it and have the value return an ERANGE (or whatever the normal point is). This also permits future library versions to extend the range change, say, up to 255 bytes (whereas you'd be trimming to 16 all the time).
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
Looks good to me. @swift-ci please test and merge |
_CFThreadSetName(): Take the thread ID, as Linux allows setting
the name of another thread. On macOS just check that it is the
same as pthread_self().
On Linux, trim the name to 15 characters as that is the maximum
that can be set.
When starting a thread, set it's name in pthreads if the name is
non-nil.
On Linux, make _thread an Optional initialised to nil instead of setting
it to pthread_t(), which gives it the value 0. This matches macOS
behaviour.
TestThread.swift: Add some more tests.