Skip to content

Thread: Fixes to thread name #1279

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 3 commits into from
Nov 6, 2017
Merged

Thread: Fixes to thread name #1279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c2585f
Thread: Fixes to thread name
spevans Oct 21, 2017
ba6bc4b
Thread: fix missing parameter name
spevans Oct 23, 2017
0021cb6
Thread: Dont trim thread name on Linux
spevans Nov 3, 2017
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
15 changes: 11 additions & 4 deletions CoreFoundation/Base.subproj/CFPlatform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1316,15 +1316,22 @@ _CFThreadRef _CFThreadCreate(const _CFThreadAttributes attrs, void *_Nullable (*
return thread;
}

CF_SWIFT_EXPORT void _CFThreadSetName(const char *_Nullable name) {
CF_SWIFT_EXPORT int _CFThreadSetName(pthread_t thread, const char *_Nonnull name) {
#if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED || DEPLOYMENT_TARGET_EMBEDDED_MINI
pthread_setname_np(name);
if (pthread_equal(pthread_self(), thread)) {
return pthread_setname_np(name);
}
return EINVAL;
#elif DEPLOYMENT_TARGET_LINUX
pthread_setname_np(pthread_self(), name);
// pthread_setname_np will fail if name >= 16 characters
char short_name[16];
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

strncpy(short_name, name, 15);
short_name[15] = '\0';
return pthread_setname_np(thread, short_name);
#endif
}

CF_SWIFT_EXPORT int _CFThreadGetName(char *buf, int length) {
CF_SWIFT_EXPORT int _CFThreadGetName(char *_Nonnull buf, int length) {
#if DEPLOYMENT_TARGET_MACOSX || DEPLOYMENT_TARGET_EMBEDDED || DEPLOYMENT_TARGET_EMBEDDED_MINI
return pthread_getname_np(pthread_self(), buf, length);
#elif DEPLOYMENT_TARGET_ANDROID
Expand Down
4 changes: 2 additions & 2 deletions CoreFoundation/Base.subproj/ForSwiftFoundationOnly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,8 @@ typedef pthread_t _CFThreadRef;

CF_EXPORT _CFThreadRef _CFThreadCreate(const _CFThreadAttributes attrs, void *_Nullable (* _Nonnull startfn)(void *_Nullable), void *restrict _Nullable context);

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we lost the 'buf' name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot, that is unintended. I will sort out a fix


CF_EXPORT Boolean _CFCharacterSetIsLongCharacterMember(CFCharacterSetRef theSet, UTF32Char theChar);
CF_EXPORT CFCharacterSetRef _CFCharacterSetCreateCopy(CFAllocatorRef alloc, CFCharacterSetRef theSet);
Expand Down
14 changes: 9 additions & 5 deletions Foundation/Thread.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ internal enum _NSThreadStatus {
private func NSThreadStart(_ context: UnsafeMutableRawPointer?) -> UnsafeMutableRawPointer? {
let thread: Thread = NSObject.unretainedReference(context!)
Thread._currentThread.set(thread)
if let name = thread.name {
_CFThreadSetName(pthread_self(), name)
}
thread._status = .executing
thread.main()
thread._status = .finished
Expand Down Expand Up @@ -141,11 +144,12 @@ open class Thread : NSObject {
}

internal var _main: () -> Void = {}
#if os(OSX) || os(iOS) || CYGWIN
private var _thread: pthread_t? = nil
#elseif os(Linux) || os(Android)
#if os(Android)
private var _thread = pthread_t()
#else
private var _thread: pthread_t? = nil
#endif

#if CYGWIN
internal var _attr : pthread_attr_t? = nil
#else
Expand Down Expand Up @@ -202,8 +206,8 @@ open class Thread : NSObject {

open var name: String? {
didSet {
if _thread == Thread.current._thread {
_CFThreadSetName(name)
if let thread = _thread {
_CFThreadSetName(thread, name ?? "" )
}
}
}
Expand Down
65 changes: 50 additions & 15 deletions TestFoundation/TestThread.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,34 +59,69 @@ class TestThread : XCTestCase {
}

func test_threadName() {
let thread = Thread()
XCTAssertNil(thread.name)

func getPThreadName() -> String? {
var buf = [Int8](repeating: 0, count: 16)
// Compare the name set in pthreads()
func compareThreadName(to name: String) {
var buf = [Int8](repeating: 0, count: 128)
#if os(OSX) || os(iOS)
// Dont use _CF functions on macOS as it will break testing with Darwin's native Foundation.
let r = pthread_getname_np(pthread_self(), &buf, buf.count)
#else
let r = _CFThreadGetName(&buf, Int32(buf.count))

guard r == 0 else {
return nil
#endif
if r == 0 {
XCTAssertEqual(String(cString: buf), name)
} else {
XCTFail("Cant get thread name")
}
return String(cString: buf)
}

// No name is set initially
XCTAssertNil(Thread.current.name)

#if os(Linux) // Linux sets the initial thread name to the process name.
compareThreadName(to: "TestFoundation")
#else
compareThreadName(to: "")
#endif
Thread.current.name = "mainThread"
XCTAssertEqual(Thread.mainThread.name, "mainThread")

let condition = NSCondition()
condition.lock()

let thread2 = Thread() {
Thread.current.name = "Thread2"
XCTAssertEqual(Thread.current.name, "Thread2")
XCTAssertEqual(Thread.current.name, getPThreadName())
XCTAssertEqual(Thread.current.name, "Thread2-1")
compareThreadName(to: "Thread2-1")

Thread.current.name = "Thread2-2"
XCTAssertEqual(Thread.current.name, "Thread2-2")
compareThreadName(to: "Thread2-2")

Thread.current.name = "12345678901234567890"
XCTAssertEqual(Thread.current.name, "12345678901234567890")
#if os(OSX) || os(iOS)
compareThreadName(to: "12345678901234567890")
#elseif os(Linux)
// pthread_setname_np() only allows 15 characters on Linux
compareThreadName(to: "123456789012345")
#endif
condition.lock()
condition.signal()
condition.unlock()
}

thread2.name = "Thread2-1"
thread2.start()

Thread.current.name = "CurrentThread"
XCTAssertEqual(Thread.current.name, getPThreadName())
// Allow 1 second for thread2 to finish
XCTAssertTrue(condition.wait(until: Date(timeIntervalSinceNow: 1)))
condition.unlock()

XCTAssertEqual(Thread.current.name, "mainThread")
XCTAssertEqual(Thread.mainThread.name, "mainThread")
let thread3 = Thread()
thread3.name = "Thread3"
XCTAssertEqual(thread3.name, "Thread3")
XCTAssertNotEqual(thread3.name, getPThreadName())
}

func test_mainThread() {
Expand Down