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

rt: use internal ThreadId implementation #5184

Closed
wants to merge 6 commits into from
Closed

Conversation

carllerche
Copy link
Member

The version provided by std has limitations, including no way to try to get a thread ID without panicking.

The version provided by `std` has limitations, including no way to try
to get a thread ID without panicking.
@carllerche carllerche added C-maintenance Category: PRs that clean code up or issues documenting cleanup. A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels Nov 10, 2022
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 10, 2022
@carllerche carllerche removed the R-loom Run loom tests on this PR label Nov 10, 2022
Comment on lines +7 to +26
cfg_has_atomic_u64! {
pub(crate) fn new() -> ThreadId {
use std::sync::atomic::{AtomicU64, Ordering::Relaxed};

static COUNTER: AtomicU64 = AtomicU64::new(0);

let mut last = COUNTER.load(Relaxed);
loop {
let id = match last.checked_add(1) {
Some(id) => id,
None => exhausted(),
};

match COUNTER.compare_exchange_weak(last, id, Relaxed, Relaxed) {
Ok(_) => return ThreadId(NonZeroU64::new(id).unwrap()),
Err(id) => last = id,
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that doing the same as in list.rs would be sufficient here.

// The id from the module below is used to verify whether a given task is stored
// in this OwnedTasks, or some other task. The counter starts at one so we can
// use zero for tasks not owned by any list.
//
// The safety checks in this file can technically be violated if the counter is
// overflown, but the checks are not supposed to ever fail unless there is a
// bug in Tokio, so we accept that certain bugs would not be caught if the two
// mixed up runtimes happen to have the same id.
cfg_has_atomic_u64! {
use std::sync::atomic::{AtomicU64, Ordering};
static NEXT_OWNED_TASKS_ID: AtomicU64 = AtomicU64::new(1);
fn get_next_id() -> u64 {
loop {
let id = NEXT_OWNED_TASKS_ID.fetch_add(1, Ordering::Relaxed);
if id != 0 {
return id;
}
}
}
}
cfg_not_has_atomic_u64! {
use std::sync::atomic::{AtomicU32, Ordering};
static NEXT_OWNED_TASKS_ID: AtomicU32 = AtomicU32::new(1);
fn get_next_id() -> u64 {
loop {
let id = NEXT_OWNED_TASKS_ID.fetch_add(1, Ordering::Relaxed);
if id != 0 {
return u64::from(id);
}
}
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We do use the thread ID for correctness on top of the debug checks. It seems riskier, in that case, to fall back to u32.

Copy link
Member

@hawkw hawkw Nov 10, 2022

Choose a reason for hiding this comment

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

IIRC std's thread IDs use a mutex on 32-bit targets, but I'd have to double check to make sure...

edit: oh, i see that that's not actually what this conversation was about, my bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we use it for correctness?

Copy link
Member Author

Choose a reason for hiding this comment

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

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 10, 2022
@carllerche carllerche removed the R-loom Run loom tests on this PR label Nov 10, 2022
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 10, 2022
@carllerche carllerche removed the R-loom Run loom tests on this PR label Nov 10, 2022
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 10, 2022
@carllerche carllerche removed the R-loom Run loom tests on this PR label Nov 10, 2022
@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 11, 2022
@carllerche carllerche removed the R-loom Run loom tests on this PR label Nov 11, 2022
@carllerche
Copy link
Member Author

Implemented by #5329

@carllerche carllerche closed this Jan 4, 2023
@carllerche carllerche deleted the clean-thread-id branch January 5, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants