iddm / thread-priority

A simple Cross-platform thread schedule and priority library for rust.
MIT License
110 stars 20 forks source link

`ThreadExt` is highly misleading #30

Open CAD97 opened 1 year ago

CAD97 commented 1 year ago

Specifically, because ThreadExt::get_native_id (and thus all of the other methods) doesn't get the native id, it gets the id of the current thread.

As an example:

let current_thread_id = std::thread::current().get_native_id();
let spawned_thread_id = std::thread::spawn(|| ()).thread().get_native_id();
assert_ne!(current_thread_id, spawned_thread_id); // panics

Or written another way:

let parent_thread = std::thread::current();
let parent_thread_id = parent_thread.get_native_id();
std::thread::spawn(|| {
    assert_eq!(parent_thread_id, parent_thread.get_native_id()); // panics
}).join().unwrap();

Removing the extension trait or adding a check that self == thread::current() is required to not be giving actively misleading results.

std::thread::JoinHandle actually supports getting the raw OS handle on cfg(windows) via AsRawHandle::as_raw_handle and cfg(unix) via JoinHandleExt::as_pthread_t. Perhaps it's not completely unreasonable to suggest that we could get similar extension trait access to the OS handle for std::thread::Thread. Until we do, though, ThreadExt is impossible to implement in the implied way of actually getting the data for the represented thread.

iddm commented 1 year ago

Hi @CAD97! Thank you for your report.

Let me answer.

  1. Thank you for pointing this out. It is indeed misleading that it doesn't work for the thread object and always returns the current thread's id. For now, I'll add a check as you suggested, as there is no other way to do that for a thread which isn't current.
  2. The name "get_native_id()" is also a bit misleading, as it is not a "native id". The "native" here may vary depending on what one might want to call "native": pthread id vs tid - what is more "native"? But I can't do anything with that, so I called id native just to avoid clashes with the already existing Thread::id().
  3. The raw handle of a JoinHandle might not be the same as the get_native_id() for the reasons I mentioned above. For now, I work with threads on UNIX via pthread and on windows via native windows threads, so it should work. But if someone expects to have a gettid() returned on Linux, this wouldn't be the case. If we have a JoinHandleExt which has JoinHandleExt::get_native_id(), then we will become dependent on pthread implementation and native windows threads and on the JoinHandle::as_raw_handle() and JoinHandle::as_pthread_t() implementation (and existence) too.

To sum up: I'll add a check and then think of how we can use (and whether or not it will be enough and good enough) the existing Rust methods.

iddm commented 1 year ago

I have an idea of creating a separate structure called ThreadExt or PrioritisedThread, which would encapsulate the std::thread::Thread inside and a native thread id (as I call it). How does that sound to you?