Skip to content
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.

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

ThreadExt is highly misleading #30

Open
CAD97 opened this issue Mar 16, 2023 · 2 comments
Open

ThreadExt is highly misleading #30

CAD97 opened this issue Mar 16, 2023 · 2 comments

Comments

@CAD97
Copy link

CAD97 commented Mar 16, 2023

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
Copy link
Owner

iddm commented Mar 16, 2023

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 added a commit that referenced this issue Mar 16, 2023
This is a part of #30

We can't get another thread's ID if we didn't store it when it was
created. As we don't have control over that, we can't properly
implement the `ThreadExt::get_native_id()` for any thread which isn't
current.
@iddm
Copy link
Owner

iddm commented Mar 20, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants