First implementation of Thread::set_name(&str)

[dns2utf8]

I would like to be able to rename threads in the threadpool crate without restarting the worker-threads.

This is my first PR with unsafe code, so I improvised a little to get it to compile.
Ideas for improvement are very appreciated.

[bluss]

Hi, there's a couple of details of the whole thread implementation I can't review but I have a few comments.

We need to remember that using unsafe does not mean that we can ignore Rust's guarantees, it only means we need to uphold them manually.

[bluss]

Mutating through a shared pointer like this is illegal, inside and outside of unsafe blocks.

For the inner thread we have an Arc<Inner> and no interior mutability, in particular no interior mutability around the cname field, so it's completely impossible to mutate that field, unless we get mutable access to it like with Arc::get_mut, but that method is only useful when there is no other handle to the same Arc.

So to make it possible to change cname, interior mutability is needed. Usually this means Mutex.

Unfortunately. The method pub fn name(&self) -> Option<&str> just a bit up in the file will exclude any possibility of using interior mutability. This is where even more creativity needs to pour in to solve it. 😄

[carols10cents]

Friendly ping @dns2utf8! Does what @bluss said make sense? Do you need any help figuring out a better solution here?

[dns2utf8]

Thank you for the input :D
Small question: How can I mark a function to execute unsafe code?

[sfackler]

What do you mean by that exactly? An unsafe fn foo() is unsafe to call, and an unsafe {...} block can call unsafe code internally.

[dtolnay]

@rust-lang/infra, how come this never got assigned by @rust-highfive?

[bluss]

@dns2utf8 I don't see how to solve it, but that doesn't mean there isn't a solution. There is no simple fix, like marking the function differently -- see my previous comments about unsafe code.

[alexcrichton]

@dtolnay I wouldn't classify @rust-highfive as the most reliable piece of software

[bluss]

Some ideas were floated with @dtolnay

• remember which names have been returned by name() and remember all of those, while freeing names that are replaced without ever being returned by name()
  • + adding an alternative to name() that returns Arc<str> or String so the name isn't marked as needing to be remembered forever
• name() -> None whenever name has changed, so name changers need to use the new (Arc<String>) accessor

One question, do the OSes all allow changing the name of a thread?

[alexcrichton]

ping @dns2utf8 would you be willing to update with @bluss's comments?

[dns2utf8]

I am. Thank you for the reminder

[bluss]

They are rather loose ideas. It seems like there is agreement ther is no "perfect" solution, so it's interesting what's pragmatically possible here and if the compromises needed are acceptable.

[alexcrichton]

@dns2utf8 looks like there may be a missing #![feature] directive in the doctest?

[alexcrichton]

Hm so reading over this and the discussion now I see how this is going to be a very difficult method to implement. @dns2utf8 you stated motivation in the OP, I wonder if it would perhaps work to configure the name on the thread builder?

I think the implementation here can also run into problems though (in addition to the mutability concerns brought up by @bluss). A Thread is sendable to other threads, so if you set the name for some other Thread on your own thread, I think it'd update the wrong OS thread name?

[dns2utf8]

I am getting an error about libc::pthread_getname_np. Is this the crate from crates.io or is it embedded into this project?
I think the function is not yet relayed and I would like to add it.

[sfackler]

You'll want to add that to the libc crate: https://github.com/rust-lang/libc.

(The review was for a previous version)

[dns2utf8]

The PR will fail until rust-lang/libc#789 is merged

[vojtechkral]

@dns2utf8
The way the code is right now it seems to me it will only compile on Linux. You'd also need to correct the support for Windows and the OSes that don't have thread names (like illumos) and also add support for various BSDs.

Give the Mac OS X limitation and the mutability/race problems mentioned by others, I wouldn't recommend trying to set the thread name via the handle. The Mac OS X limitation won't be fixed anytime soon IMHO.

Basically the options are:

• Either only provide functions to set/get the current running thread name, if supported by the OS. I like this approach the best as it works on the most OSes and is simple to implement. (Also deprecate Thread::name() since it's not really realiable anyway even today.)
• Or implement the name setting on the handle, but leave out OS X as unsupported and figure out how to not run into race conditions, etc. Much more complex and no OS X support.

I'd say the first approach should be fine when writing a threapool since you're probably going to need a way to communicate with your threads anyway, right?

[dns2utf8]

Thank you all for the comments. I am sorry, about never writing back.
So let me answer what I currently can:

• OS support: As far as I can tell all tier 1 (Linux, MacX and Windows) support setting the name for the own thread and reading the name via a handle. I don't know if apple has plans for adding renaming with a handle.
• deprecate Thread::name(): I agree. It uses CStr in the background which limits the API a lot.
• use an Arc: I would like to avoid Mutex or Arc if possible. Maybe I will have to add an AtomicPointer for backwards compatibility, but we will see.
• tier 2 platforms: I plan to work on them after tier 1 works.

My current plan is as follows:

• getting the name: ask the OS for the name every time and return an owned OsString (or maybe String if there are good reasons for that). Since the name can change at any time (FFI, the kernel or even an other program) it is more reliable to fetch it from the only source that knows. Plus this elides scenarios like when the thread ends there is no bookkeeping or lifetime issue about the slice.
• setting the name: on most platforms the operation itself is thread safe. Even if it was implemented with the handler, the latest one would win and the behavior is well defined. Since MacX is currently blocking that approach @alexcrichton proposed a free function like thread::current(). This prevents races and lifetime problems but in the end a the thread can rename itself only.

I am currently thinking about wrapping the result from get into a Result to catch cases like A obtains a handle, B finishes, A asks for the name -> Err(...).
However I need some days to recover from RustFest.eu so I will continues in a couple of days.

Again, thank you all for the comments, they helped me 👍

[alexcrichton]

@dns2utf8 oh I was thinking that for fetching the name we probably won't query the OS 100% of the time but would instead use what we have today with a stored value in Thread, but that value would get updated whenever the free "set name" function is called for sure. Either way works though!

[sfackler]

At least on pthreads, the OS will truncate the thread name to ~16 bytes, so we'd want to store a copy locally instead of relying on the pthreads API.

[vojtechkral]

@sfackler Well, returning the truncated name would better reflect what the thread is actually named, no? I guess this boils down to whether the thread name should be useful for 'outside' interaction (ie. looking up a thread in a process list / manager, etc.), or to be useful for internal purposes of the program.

edit: Also, a call to 3rd party and/or native API might change the thread name, in which case the cached name would no longer reflect the actual name.

[sfackler]

The thread name is used quite often inside of the program - for example, it's often attached to log output. There, you'd absolutely want to see the full name rather than just the first 16 bytes. Web server's commonly name the thread handling a request with e.g. POST /api/v1/foobar/fizzbuz?baz=2. If you're logging messages, you want to see that full thing rather than POST /api/v1/foo.

[vojtechkral]

@sfackler Ok, that makes sense, but then you either need access to the thread handle whenever you log something or store the thread name elsewhere, such as TLS (?).

[sfackler]

It's easy to get access to the handle for the thread you're on: thread::current().

[dns2utf8]

From my point of view, the first implementation should query the OS every time.
In the second step I can imagine a form of fingerprinting the OS on plattfroms with short names because this

• will reflect changes done by other programms or FFI
• does not require a lock or any synchronisation

[bluss]

r? @sfackler

I hope you don't mind — the OS details are not my area.

[sfackler]

does not require a lock or any synchronisation

I'm not sure what you mean by this. There is both locking and synchronization being performed in pthread_getname_np.

[alexcrichton]

Triage ping for @bluss! Do you have thoughts on the failing tests or @sfackler's recent comment?

[alexcrichton]

er sorry, that was for @dns2utf8

[kennytm]

Hi @dns2utf8, are you still working on this? 😃

[shepmaster]

Thanks for the PR, @dns2utf8, but we haven't heard from you in a few weeks. I am going to close this PR to keep things tidy. Please feel free to reopen it once you've addressed the most recent feedback!