Rust: thread_id_value tracking issue

[Mark-Simulacrum]

This is the tracking issue for the ThreadId::as_u64 method, which casts a thread ID to the underlying u64.

There are currently no known blockers to stabilization beyond making sure that the API indeed works for intended use cases.

Implemented originally in https://github.com/rust-lang/rust/pull/67566.

[brain0]

Shouldn't this at least guarantee that a valid id is never 0?

[Mark-Simulacrum]

Why would you expect such a guarantee?

We could clearly make it, but it seems a bit odd to me for us to do so...

[brain0]

Such a guarantee would allow storing the equivalent of Option<ThreadId> in an AtomicU64.

I have a case where I need to avoid some recursive operation: So, a thread starts the operation (by executing an FnOnce() passed from outside) and saves its thread id. If the same thread performs the same operation again recursively, I panic. If another thread does it, I park the thread. In this case, 0 would serve as a value indicating "no thread has been assigned yet".

The current implementation stores the ThreadId in NonZeroU64, so if we can make a stable guarantee that a thread id will never be 0, returning NonZeroU64 instead of u64 here would help a lot.

[Mark-Simulacrum]

I think I would personally prefer to avoid using NonZero in this API. I'd suggest manually doing so in the consumer of the API, perhaps assigning u64::MAX instead of 0 as the sentinel for None.

[brain0]

Without any guarantee that a thread id cannot be u64::MAX that is not a good idea. And the current state is "there are no guarantees".

[Mark-Simulacrum]

Well, sure, I guess, though realistically it's the sort of thing that would be fine to do IMO and panic on -- you're almost certainly not going to hit that.

I'm not opposed to accepting this, I guess -- would you be willing to file a PR changing the API as you've suggested?

[brain0]

Well, sure, I guess, though realistically it's the sort of thing that would be fine to do IMO and panic on -- you're almost certainly not going to hit that.

I always prefer a static guarantee (as_u64 returning a type that guarantees an invariant) over "almost certainly fine".

I'm not opposed to accepting this, I guess -- would you be willing to file a PR changing the API as you've suggested?

Sure, I can do that. But I still don't quite understand yet why you don't like it the idea.

[Mark-Simulacrum]

It's not really that I don't like it -- I think you've convinced me that it can be useful and is pretty minimal -- but I was uncertain about making the guarantee going forward. But thinking about it more, 64 bits should be more than enough that we shouldn't need to worry about the zero being lost to us if we make the guarantee.

[Amanieu]

This API should return a usize, not a u64. AtomicU64 isn't supported on all platforms, while AtomicUsize is. Also since threads share the address space, it should be possible to guarantee that an id fits in a usize.

[Mark-Simulacrum]

@Amanieu We currently use a u64 I think because (a sufficiently long lived) process could spawn more than 2^32 threads, not simultaneously. And currently we guarantee that the index is unique, i.e. will not wrap. I am unopposed to changing this, but we should do so in ThreadId::new and make a decision about behavior on wrapping.

[joshtriplett]

There's value in not reusing the IDs, such as if you use them as unique identifiers for log files.